diff mbox

m25p80: Use a 512 byte page size for Spansion flash s25fl512s

Message ID 1485265929-24020-1-git-send-email-mark.marshall@omicronenergy.com
State Changes Requested
Headers show

Commit Message

mark.marshall@omicronenergy.com Jan. 24, 2017, 1:52 p.m. UTC 
From: Mark Marshall <mark.marshall@omicronenergy.com>

The s25fl512s flash from Spansion has a 512 byte write page size,
which means that we can write 512 bytes at a time (instead of 256).

This single change makes writing to the flash about 2x faster.

Signed-off-by: Mark Marshall <mark.marshall@omicronenergy.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

Comments

Marek Vasut Jan. 24, 2017, 4:48 p.m. UTC | #1 
On 01/24/2017 02:52 PM, mark.marshall@omicronenergy.com wrote:
> From: Mark Marshall <mark.marshall@omicronenergy.com>
> 
> The s25fl512s flash from Spansion has a 512 byte write page size,
> which means that we can write 512 bytes at a time (instead of 256).
> 
> This single change makes writing to the flash about 2x faster.
> 
> Signed-off-by: Mark Marshall <mark.marshall@omicronenergy.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index da7cd69..c9ac0bf 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -775,6 +775,21 @@ static int spi_nor_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>  		.page_size = 256,					\
>  		.flags = (_flags),
>  
> +/* Used to set a custom (non 256) page_size */
> +#define INFOP(_jedec_id, _ext_id, _sector_size, _n_sectors, _pg_sz, _flags) \
> +		.id = {							\
> +			((_jedec_id) >> 16) & 0xff,			\
> +			((_jedec_id) >> 8) & 0xff,			\
> +			(_jedec_id) & 0xff,				\
> +			((_ext_id) >> 8) & 0xff,			\
> +			(_ext_id) & 0xff,				\
> +			},						\
> +		.id_len = (!(_jedec_id) ? 0 : (3 + ((_ext_id) ? 2 : 0))), \
> +		.sector_size = (_sector_size),				\
> +		.n_sectors = (_n_sectors),				\
> +		.page_size = (_pg_sz),					\
> +		.flags = (_flags),					\
> +

Maybe it's time to introduce INFO_FULL() instead, where you could
specify the page size and ID length. Adding more and more custom INFOx()
doesn't scale. The INFOx() could then be easily converted
over to INFO_FULL().

One minor bit which could be slightly problematic is the .id_len
field, which is precomputed now, but maybe there is a way to handle
that too.

Thoughts ?

>  #define CAT25_INFO(_sector_size, _n_sectors, _page_size, _addr_width, _flags)	\
>  		.sector_size = (_sector_size),				\
>  		.n_sectors = (_n_sectors),				\
> @@ -905,7 +920,7 @@ static const struct flash_info spi_nor_ids[] = {
>  	{ "s25sl064p",  INFO(0x010216, 0x4d00,  64 * 1024, 128, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
>  	{ "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, 0) },
>  	{ "s25fl256s1", INFO(0x010219, 0x4d01,  64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> -	{ "s25fl512s",  INFO(0x010220, 0x4d00, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> +	{ "s25fl512s",  INFOP(0x010220, 0x4d00, 256 * 1024, 256, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
>  	{ "s70fl01gs",  INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) },
>  	{ "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
>  	{ "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },
>
Mark Marshall Jan. 26, 2017, 2:58 p.m. UTC | #2 
On 24 January 2017 at 17:48, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 01/24/2017 02:52 PM, mark.marshall@omicronenergy.com wrote:
>> From: Mark Marshall <mark.marshall@omicronenergy.com>
>>
>> The s25fl512s flash from Spansion has a 512 byte write page size,
>> which means that we can write 512 bytes at a time (instead of 256).
>>
>> This single change makes writing to the flash about 2x faster.
>>
>> Signed-off-by: Mark Marshall <mark.marshall@omicronenergy.com>
>> ---
>>  drivers/mtd/spi-nor/spi-nor.c | 17 ++++++++++++++++-
>>  1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index da7cd69..c9ac0bf 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -775,6 +775,21 @@ static int spi_nor_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>>               .page_size = 256,                                       \
>>               .flags = (_flags),
>>
>> +/* Used to set a custom (non 256) page_size */
>> +#define INFOP(_jedec_id, _ext_id, _sector_size, _n_sectors, _pg_sz, _flags) \
>> +             .id = {                                                 \
>> +                     ((_jedec_id) >> 16) & 0xff,                     \
>> +                     ((_jedec_id) >> 8) & 0xff,                      \
>> +                     (_jedec_id) & 0xff,                             \
>> +                     ((_ext_id) >> 8) & 0xff,                        \
>> +                     (_ext_id) & 0xff,                               \
>> +                     },                                              \
>> +             .id_len = (!(_jedec_id) ? 0 : (3 + ((_ext_id) ? 2 : 0))), \
>> +             .sector_size = (_sector_size),                          \
>> +             .n_sectors = (_n_sectors),                              \
>> +             .page_size = (_pg_sz),                                  \
>> +             .flags = (_flags),                                      \
>> +
>
> Maybe it's time to introduce INFO_FULL() instead, where you could
> specify the page size and ID length. Adding more and more custom INFOx()
> doesn't scale. The INFOx() could then be easily converted
> over to INFO_FULL().
>
> One minor bit which could be slightly problematic is the .id_len
> field, which is precomputed now, but maybe there is a way to handle
> that too.
>
> Thoughts ?

I had a go at this, and my preferred method would be to use some token
pasting, but I'm not sure how popular this will be?  Below is the macro's I
came up with and a few representative lines of the table.  If no one objects
I'll prepare a complete patch.  (I've done this on a branch, and the table has
changed slightly, where should I be basing a patch like this from?).

I've checked by building the file both before and after my change, and the
table doesn't change.

#define _ID_NONE                \
    .id = {},                \
    .id_len = 0

#define _ID_JEDEC(_jedec_id)                        \
    .id = {                                \
        ((_jedec_id) >> 16) & 0xff,                \
        ((_jedec_id) >> 8) & 0xff,                \
        (_jedec_id) & 0xff,                    \
    },                                \
    .id_len = 3

#define _ID_JEDEC_EXT2(_jedec_id, _ext_id)                \
    .id = {                                \
        ((_jedec_id) >> 16) & 0xff,                \
        ((_jedec_id) >> 8) & 0xff,                \
        (_jedec_id) & 0xff,                    \
        ((_ext_id) >> 8) & 0xff,                \
        (_ext_id) & 0xff,                    \
    },                                \
    .id_len = 5

#define _ID_JEDEC_EXT3(_jedec_id, _ext_id)                \
    .id = {                                \
        ((_jedec_id) >> 16) & 0xff,                \
        ((_jedec_id) >> 8) & 0xff,                \
        (_jedec_id) & 0xff,                    \
        ((_ext_id) >> 16) & 0xff,                \
        ((_ext_id) >> 8) & 0xff,                \
        (_ext_id) & 0xff,                    \
    },                                \
    .id_len = 6

#define INFO_FULL(_id, _sector_size, _n_sectors, _pg_sz, _addr_width, _flags) \
    _ID_ ## _id,                            \
    .sector_size = (_sector_size),                    \
    .n_sectors = (_n_sectors),                    \
    .page_size = (_pg_sz),                        \
    .addr_width = (_addr_width),                    \
    .flags = (_flags),

#define INFO(_id, _sector_size, _n_sectors, _flags)    \
    INFO_FULL(_id, _sector_size, _n_sectors, 256, 0, _flags)

static const struct flash_info spi_nor_ids[] = {
    { "at25fs010",  INFO(JEDEC(0x1f6601), 32 * 1024,   4, SECT_4K) },
    { "at25fs040",  INFO(JEDEC(0x1f6604), 64 * 1024,   8, SECT_4K) },
    { "mr25h256", INFO_FULL(NONE,  32 * 1024, 1, 256, 2,
SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
    { "s25sl032p",  INFO(JEDEC_EXT2(0x010215, 0x4d00),  64 * 1024,
64, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
    { "s25fl512s",  INFO_FULL(JEDEC_EXT2(0x010220, 0x4d00), 256 *
1024, 256, 256, 0, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
    { "s25fl128s",    INFO(JEDEC_EXT3(0x012018, 0x4d0180), 64 * 1024,
256, SECT_4K | SPI_NOR_QUAD_READ) },
    { "cat25c11", INFO_FULL(NONE,   16, 8, 16, 1, SPI_NOR_NO_ERASE |
SPI_NOR_NO_FR) },

Regards,

Mark
Marek Vasut Feb. 4, 2017, 10:25 p.m. UTC | #3 
On 01/26/2017 03:58 PM, Mark Marshall wrote:
> On 24 January 2017 at 17:48, Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 01/24/2017 02:52 PM, mark.marshall@omicronenergy.com wrote:
>>> From: Mark Marshall <mark.marshall@omicronenergy.com>
>>>
>>> The s25fl512s flash from Spansion has a 512 byte write page size,
>>> which means that we can write 512 bytes at a time (instead of 256).
>>>
>>> This single change makes writing to the flash about 2x faster.
>>>
>>> Signed-off-by: Mark Marshall <mark.marshall@omicronenergy.com>
>>> ---
>>>  drivers/mtd/spi-nor/spi-nor.c | 17 ++++++++++++++++-
>>>  1 file changed, 16 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>>> index da7cd69..c9ac0bf 100644
>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>> @@ -775,6 +775,21 @@ static int spi_nor_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>>>               .page_size = 256,                                       \
>>>               .flags = (_flags),
>>>
>>> +/* Used to set a custom (non 256) page_size */
>>> +#define INFOP(_jedec_id, _ext_id, _sector_size, _n_sectors, _pg_sz, _flags) \
>>> +             .id = {                                                 \
>>> +                     ((_jedec_id) >> 16) & 0xff,                     \
>>> +                     ((_jedec_id) >> 8) & 0xff,                      \
>>> +                     (_jedec_id) & 0xff,                             \
>>> +                     ((_ext_id) >> 8) & 0xff,                        \
>>> +                     (_ext_id) & 0xff,                               \
>>> +                     },                                              \
>>> +             .id_len = (!(_jedec_id) ? 0 : (3 + ((_ext_id) ? 2 : 0))), \
>>> +             .sector_size = (_sector_size),                          \
>>> +             .n_sectors = (_n_sectors),                              \
>>> +             .page_size = (_pg_sz),                                  \
>>> +             .flags = (_flags),                                      \
>>> +
>>
>> Maybe it's time to introduce INFO_FULL() instead, where you could
>> specify the page size and ID length. Adding more and more custom INFOx()
>> doesn't scale. The INFOx() could then be easily converted
>> over to INFO_FULL().
>>
>> One minor bit which could be slightly problematic is the .id_len
>> field, which is precomputed now, but maybe there is a way to handle
>> that too.
>>
>> Thoughts ?

Sorry for the late reply, I've been traveling a lot recently.

> I had a go at this, and my preferred method would be to use some token
> pasting, but I'm not sure how popular this will be?

IMO not much.

> Below is the macro's I
> came up with and a few representative lines of the table.  If no one objects
> I'll prepare a complete patch.  (I've done this on a branch, and the table has
> changed slightly, where should I be basing a patch like this from?).

This makes me kinda wonder (and this might be a totally wrong idea), why
don't we replace the 6-byte ID table with an u64 id and u64 mask ?
_ID_JEDEC_EXT2 would then look something like:

#define _ID_JEDEC_EXT2(_jedec_id, _ext_id)		\
{							\
	.id = ((_ext_id) << 32) | (_jedec_id),		\
	.mask = GENMASK(47, 32) | GENMASK(23, 0),	\
},

The match would then also be much easier, that is.
->id & ->mask == ->id
instead of all the memcmp() tests. id_len would go
away too. And the matching would allow more precise
control over the test. We could even handle the n25q256(a)
specialty jedec ID 0x20baxx/0x20bbxx with this.

I don't think there'll ever be a SPI NOR with 8byte JEDEC
ID, so u64 should be good enough.

The macros below could be reworked to use a generic macro
to define the ID/extID entry and it's width, ie:

#define _ID_JEDEC(_jedec_id, _jedec_id_width, 			\
		  _ext_id, _ext_id_width)			\
{								\
	.id = ((_ext_id) << 32) | (_jedec_id),			\
	.mask = GENMASK(32 + ((_ext_id_width * 8) - 1), 32) |	\
		GENMASK(((_id_width * 8) - 1), 0),		\
},

> I've checked by building the file both before and after my change, and the
> table doesn't change.
> 
> #define _ID_NONE                \
>     .id = {},                \
>     .id_len = 0
> 
> #define _ID_JEDEC(_jedec_id)                        \
>     .id = {                                \
>         ((_jedec_id) >> 16) & 0xff,                \
>         ((_jedec_id) >> 8) & 0xff,                \
>         (_jedec_id) & 0xff,                    \
>     },                                \
>     .id_len = 3
> 
> #define _ID_JEDEC_EXT2(_jedec_id, _ext_id)                \
>     .id = {                                \
>         ((_jedec_id) >> 16) & 0xff,                \
>         ((_jedec_id) >> 8) & 0xff,                \
>         (_jedec_id) & 0xff,                    \
>         ((_ext_id) >> 8) & 0xff,                \
>         (_ext_id) & 0xff,                    \
>     },                                \
>     .id_len = 5
> 
> #define _ID_JEDEC_EXT3(_jedec_id, _ext_id)                \
>     .id = {                                \
>         ((_jedec_id) >> 16) & 0xff,                \
>         ((_jedec_id) >> 8) & 0xff,                \
>         (_jedec_id) & 0xff,                    \
>         ((_ext_id) >> 16) & 0xff,                \
>         ((_ext_id) >> 8) & 0xff,                \
>         (_ext_id) & 0xff,                    \
>     },                                \
>     .id_len = 6
> 
> #define INFO_FULL(_id, _sector_size, _n_sectors, _pg_sz, _addr_width, _flags) \
>     _ID_ ## _id,                            \
>     .sector_size = (_sector_size),                    \
>     .n_sectors = (_n_sectors),                    \
>     .page_size = (_pg_sz),                        \
>     .addr_width = (_addr_width),                    \
>     .flags = (_flags),
> 
> #define INFO(_id, _sector_size, _n_sectors, _flags)    \
>     INFO_FULL(_id, _sector_size, _n_sectors, 256, 0, _flags)
> 
> static const struct flash_info spi_nor_ids[] = {
>     { "at25fs010",  INFO(JEDEC(0x1f6601), 32 * 1024,   4, SECT_4K) },
>     { "at25fs040",  INFO(JEDEC(0x1f6604), 64 * 1024,   8, SECT_4K) },
>     { "mr25h256", INFO_FULL(NONE,  32 * 1024, 1, 256, 2,
> SPI_NOR_NO_ERASE | SPI_NOR_NO_FR) },
>     { "s25sl032p",  INFO(JEDEC_EXT2(0x010215, 0x4d00),  64 * 1024,
> 64, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
>     { "s25fl512s",  INFO_FULL(JEDEC_EXT2(0x010220, 0x4d00), 256 *
> 1024, 256, 256, 0, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
>     { "s25fl128s",    INFO(JEDEC_EXT3(0x012018, 0x4d0180), 64 * 1024,
> 256, SECT_4K | SPI_NOR_QUAD_READ) },
>     { "cat25c11", INFO_FULL(NONE,   16, 8, 16, 1, SPI_NOR_NO_ERASE |
> SPI_NOR_NO_FR) },
> 
> Regards,
> 
> Mark
>
diff mbox

Patch

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index da7cd69..c9ac0bf 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -775,6 +775,21 @@  static int spi_nor_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 		.page_size = 256,					\
 		.flags = (_flags),
 
+/* Used to set a custom (non 256) page_size */
+#define INFOP(_jedec_id, _ext_id, _sector_size, _n_sectors, _pg_sz, _flags) \
+		.id = {							\
+			((_jedec_id) >> 16) & 0xff,			\
+			((_jedec_id) >> 8) & 0xff,			\
+			(_jedec_id) & 0xff,				\
+			((_ext_id) >> 8) & 0xff,			\
+			(_ext_id) & 0xff,				\
+			},						\
+		.id_len = (!(_jedec_id) ? 0 : (3 + ((_ext_id) ? 2 : 0))), \
+		.sector_size = (_sector_size),				\
+		.n_sectors = (_n_sectors),				\
+		.page_size = (_pg_sz),					\
+		.flags = (_flags),					\
+
 #define CAT25_INFO(_sector_size, _n_sectors, _page_size, _addr_width, _flags)	\
 		.sector_size = (_sector_size),				\
 		.n_sectors = (_n_sectors),				\
@@ -905,7 +920,7 @@  static const struct flash_info spi_nor_ids[] = {
 	{ "s25sl064p",  INFO(0x010216, 0x4d00,  64 * 1024, 128, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
 	{ "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, 0) },
 	{ "s25fl256s1", INFO(0x010219, 0x4d01,  64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
-	{ "s25fl512s",  INFO(0x010220, 0x4d00, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
+	{ "s25fl512s",  INFOP(0x010220, 0x4d00, 256 * 1024, 256, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
 	{ "s70fl01gs",  INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) },
 	{ "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
 	{ "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },