linux-mtd.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] introduce SNOR_ID3()
@ 2022-05-10 14:02 Michael Walle
  2022-05-10 14:02 ` [PATCH 1/2] mtd: spi-nor: " Michael Walle
                   ` (2 more replies)
  0 siblings, 3 replies; 22+ messages in thread
From: Michael Walle @ 2022-05-10 14:02 UTC (permalink / raw)
  To: Tudor Ambarus, Pratyush Yadav, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Shaik Sajida Bhanu
  Cc: linux-mtd, linux-kernel, Michael Walle

For support which rely solely on the SFDP data, all we need to know is its
JEDEC ID. All further information should be fetched from SFDP. For this,
introduce a SNOR_ID3() macro which should be used for all new flashes with
SFDP.

The dropped information are page size, number of sectors and sector size.
The number of sectors (together with sector size) is only used to calculate
the total flash size. In case of SFDP, this is read from there. The sector
size is also used to select a proper erase opcode. We can just keep that
one zero and the selection algorithm will select the same erase opcode;
that is the opcode for the largest erase size (which is btw rather odd..).
The page size is also taken from SFDP. There is one exception: older SFDP
tables. These doesn't contain this data. So if there will be flashes with
that, we should assume some sane value.

Shaik, could you please re-test your w25q512nwm support with this patch. I
read through the SFDP data and it should behave exactly the same as before.

Michael Walle (2):
  mtd: spi-nor: introduce SNOR_ID3()
  mtd: spi-nor: winbond: use SNOR_ID3() for w25q512nwm

 drivers/mtd/spi-nor/core.c    | 7 +++++--
 drivers/mtd/spi-nor/core.h    | 9 +++++++++
 drivers/mtd/spi-nor/winbond.c | 3 +--
 3 files changed, 15 insertions(+), 4 deletions(-)

-- 
2.30.2


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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3()
  2022-05-10 14:02 [PATCH 0/2] introduce SNOR_ID3() Michael Walle
@ 2022-05-10 14:02 ` Michael Walle
  2022-06-05 15:00   ` Tom Fitzhenry
                     ` (2 more replies)
  2022-05-10 14:02 ` [PATCH 2/2] mtd: spi-nor: winbond: use SNOR_ID3() for w25q512nwm Michael Walle
  2022-05-10 14:03 ` [PATCH 0/2] mtd: spi-nor: introduce SNOR_ID3() Michael Walle
  2 siblings, 3 replies; 22+ messages in thread
From: Michael Walle @ 2022-05-10 14:02 UTC (permalink / raw)
  To: Tudor Ambarus, Pratyush Yadav, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Shaik Sajida Bhanu
  Cc: linux-mtd, linux-kernel, Michael Walle

Up until now, flashes were defined by specifying the JEDEC ID, the
sector size and the number of sectors. This can be read by parsing the
SFDP, we don't have to specify it. Thus provide a new macro SNOR_ID3()
which just takes the JEDEC ID and implicitly set .parse_sfdp = true. All
new flashes which have SFDP should use this macro.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/core.c | 7 +++++--
 drivers/mtd/spi-nor/core.h | 9 +++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 402b37cdbcea..29329ed0a934 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2104,8 +2104,11 @@ static int spi_nor_select_pp(struct spi_nor *nor,
  * spi_nor_select_uniform_erase() - select optimum uniform erase type
  * @map:		the erase map of the SPI NOR
  * @wanted_size:	the erase type size to search for. Contains the value of
- *			info->sector_size or of the "small sector" size in case
- *			CONFIG_MTD_SPI_NOR_USE_4K_SECTORS is defined.
+ *			info->sector_size, the "small sector" size in case
+ *			CONFIG_MTD_SPI_NOR_USE_4K_SECTORS is defined or 0 if
+ *			there is no information about the sector size. The
+ *			latter is the case if the flash parameters are parsed
+ *			solely by SFDP.
  *
  * Once the optimum uniform sector erase command is found, disable all the
  * other.
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 61886868cd02..fab3038c4f4a 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -563,6 +563,15 @@ struct flash_info {
 			.n_regions = (_n_regions),			\
 		},
 
+#define SNOR_ID3(_jedec_id)						\
+		.id = {							\
+			((_jedec_id) >> 16) & 0xff,			\
+			((_jedec_id) >> 8) & 0xff,			\
+			(_jedec_id) & 0xff,				\
+			},						\
+		.id_len = 3,						\
+		.parse_sfdp = true,					\
+
 #define PARSE_SFDP							\
 	.parse_sfdp = true,						\
 
-- 
2.30.2


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

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 2/2] mtd: spi-nor: winbond: use SNOR_ID3() for w25q512nwm
  2022-05-10 14:02 [PATCH 0/2] introduce SNOR_ID3() Michael Walle
  2022-05-10 14:02 ` [PATCH 1/2] mtd: spi-nor: " Michael Walle
@ 2022-05-10 14:02 ` Michael Walle
  2022-07-12  8:40   ` Tudor.Ambarus
  2022-05-10 14:03 ` [PATCH 0/2] mtd: spi-nor: introduce SNOR_ID3() Michael Walle
  2 siblings, 1 reply; 22+ messages in thread
From: Michael Walle @ 2022-05-10 14:02 UTC (permalink / raw)
  To: Tudor Ambarus, Pratyush Yadav, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Shaik Sajida Bhanu
  Cc: linux-mtd, linux-kernel, Michael Walle

Use the new SNOR_ID3() so we don't have to specify the number of sectors
as we are reading that property from the SFDP anyways.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/winbond.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
index 08f9b5abf4d2..12b38f6776f4 100644
--- a/drivers/mtd/spi-nor/winbond.c
+++ b/drivers/mtd/spi-nor/winbond.c
@@ -134,8 +134,7 @@ static const struct flash_info winbond_nor_parts[] = {
 	{ "w25m512jv", INFO(0xef7119, 0, 64 * 1024, 1024)
 		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ |
 			      SPI_NOR_DUAL_READ) },
-	{ "w25q512nwm", INFO(0xef8020, 0, 64 * 1024, 1024)
-		PARSE_SFDP
+	{ "w25q512nwm", SNOR_ID3(0xef8020)
 		OTP_INFO(256, 3, 0x1000, 0x1000) },
 	{ "w25q512jvq", INFO(0xef4020, 0, 64 * 1024, 1024)
 		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ |
-- 
2.30.2


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

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH 0/2] mtd: spi-nor: introduce SNOR_ID3()
  2022-05-10 14:02 [PATCH 0/2] introduce SNOR_ID3() Michael Walle
  2022-05-10 14:02 ` [PATCH 1/2] mtd: spi-nor: " Michael Walle
  2022-05-10 14:02 ` [PATCH 2/2] mtd: spi-nor: winbond: use SNOR_ID3() for w25q512nwm Michael Walle
@ 2022-05-10 14:03 ` Michael Walle
  2 siblings, 0 replies; 22+ messages in thread
From: Michael Walle @ 2022-05-10 14:03 UTC (permalink / raw)
  To: Tudor Ambarus, Pratyush Yadav, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Shaik Sajida Bhanu
  Cc: linux-mtd, linux-kernel

[proper subject]

Am 2022-05-10 16:02, schrieb Michael Walle:
> For support which rely solely on the SFDP data, all we need to know is 
> its
> JEDEC ID. All further information should be fetched from SFDP. For 
> this,
> introduce a SNOR_ID3() macro which should be used for all new flashes 
> with
> SFDP.
> 
> The dropped information are page size, number of sectors and sector 
> size.
> The number of sectors (together with sector size) is only used to 
> calculate
> the total flash size. In case of SFDP, this is read from there. The 
> sector
> size is also used to select a proper erase opcode. We can just keep 
> that
> one zero and the selection algorithm will select the same erase opcode;
> that is the opcode for the largest erase size (which is btw rather 
> odd..).
> The page size is also taken from SFDP. There is one exception: older 
> SFDP
> tables. These doesn't contain this data. So if there will be flashes 
> with
> that, we should assume some sane value.
> 
> Shaik, could you please re-test your w25q512nwm support with this 
> patch. I
> read through the SFDP data and it should behave exactly the same as 
> before.
> 
> Michael Walle (2):
>   mtd: spi-nor: introduce SNOR_ID3()
>   mtd: spi-nor: winbond: use SNOR_ID3() for w25q512nwm
> 
>  drivers/mtd/spi-nor/core.c    | 7 +++++--
>  drivers/mtd/spi-nor/core.h    | 9 +++++++++
>  drivers/mtd/spi-nor/winbond.c | 3 +--
>  3 files changed, 15 insertions(+), 4 deletions(-)

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3()
  2022-05-10 14:02 ` [PATCH 1/2] mtd: spi-nor: " Michael Walle
@ 2022-06-05 15:00   ` Tom Fitzhenry
  2022-07-12  7:23   ` Pratyush Yadav
  2022-07-19  5:57   ` Tudor.Ambarus
  2 siblings, 0 replies; 22+ messages in thread
From: Tom Fitzhenry @ 2022-06-05 15:00 UTC (permalink / raw)
  To: Michael Walle
  Cc: Tudor Ambarus, Pratyush Yadav, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Shaik Sajida Bhanu, linux-mtd, linux-kernel

In "Add support for the siliconkaiser sk25lp128 chip."[0], I have tested
that this patch works on the sk25lp128.

Thanks!

Tested-by: Tom Fitzhenry <tom@tom-fitzhenry.me.uk>

0. https://lore.kernel.org/all/20220603141603.145777-1-tom@tom-fitzhenry.me.uk/

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3()
  2022-05-10 14:02 ` [PATCH 1/2] mtd: spi-nor: " Michael Walle
  2022-06-05 15:00   ` Tom Fitzhenry
@ 2022-07-12  7:23   ` Pratyush Yadav
  2022-07-15 12:19     ` Biju Das
  2022-07-19  5:57   ` Tudor.Ambarus
  2 siblings, 1 reply; 22+ messages in thread
From: Pratyush Yadav @ 2022-07-12  7:23 UTC (permalink / raw)
  To: Michael Walle
  Cc: Tudor Ambarus, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Shaik Sajida Bhanu, linux-mtd, linux-kernel

On 10/05/22 04:02PM, Michael Walle wrote:
> Up until now, flashes were defined by specifying the JEDEC ID, the
> sector size and the number of sectors. This can be read by parsing the
> SFDP, we don't have to specify it. Thus provide a new macro SNOR_ID3()
> which just takes the JEDEC ID and implicitly set .parse_sfdp = true. All
> new flashes which have SFDP should use this macro.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Pratyush Yadav <p.yadav@ti.com>

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/2] mtd: spi-nor: winbond: use SNOR_ID3() for w25q512nwm
  2022-05-10 14:02 ` [PATCH 2/2] mtd: spi-nor: winbond: use SNOR_ID3() for w25q512nwm Michael Walle
@ 2022-07-12  8:40   ` Tudor.Ambarus
  2022-07-18  7:21     ` Michael Walle
  2022-07-18  7:25     ` Michael Walle
  0 siblings, 2 replies; 22+ messages in thread
From: Tudor.Ambarus @ 2022-07-12  8:40 UTC (permalink / raw)
  To: michael, p.yadav, miquel.raynal, richard, vigneshr, quic_c_sbhanu
  Cc: linux-mtd, linux-kernel

Shaik, can we have your Tested-by tag on this?

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3()
  2022-07-12  7:23   ` Pratyush Yadav
@ 2022-07-15 12:19     ` Biju Das
  0 siblings, 0 replies; 22+ messages in thread
From: Biju Das @ 2022-07-15 12:19 UTC (permalink / raw)
  To: michael
  Cc: linux-kernel, linux-mtd, miquel.raynal, p.yadav, quic_c_sbhanu,
	richard, tudor.ambarus, vigneshr, Biju Das

On 10/05/22 04:02PM, Michael Walle wrote:
> Up until now, flashes were defined by specifying the JEDEC ID, the
> sector size and the number of sectors. This can be read by parsing the
> SFDP, we don't have to specify it. Thus provide a new macro SNOR_ID3()
> which just takes the JEDEC ID and implicitly set .parse_sfdp = true. All
> new flashes which have SFDP should use this macro.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Tested the macro with Renesas AT25QL128A serial nor flash. So,

Tested-by: Biju Das <biju.das.jz@bp.renesas.com>

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/2] mtd: spi-nor: winbond: use SNOR_ID3() for w25q512nwm
  2022-07-12  8:40   ` Tudor.Ambarus
@ 2022-07-18  7:21     ` Michael Walle
  2022-07-18  7:25     ` Michael Walle
  1 sibling, 0 replies; 22+ messages in thread
From: Michael Walle @ 2022-07-18  7:21 UTC (permalink / raw)
  To: Tudor.Ambarus
  Cc: p.yadav, miquel.raynal, richard, vigneshr, quic_c_sbhanu,
	linux-mtd, linux-kernel

Am 2022-07-12 10:40, schrieb Tudor.Ambarus@microchip.com:
> Shaik, can we have your Tested-by tag on this?

This will need the following patch to work correctly:
https://lore.kernel.org/linux-mtd/20220716000643.3541839-1-quic_jaehyoo@quicinc.com/

But other that that, the SFDP data is exactly the same
as in the w25q512nwq, except for the "hello world" string.
But that string is between the header and the tables in
an unused area and I suspect it was accidentally written
during OTP testing as it happend with the w25q512nwq, too.

So this should be good to go once the w25q512nwq is merged.

-michael

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/2] mtd: spi-nor: winbond: use SNOR_ID3() for w25q512nwm
  2022-07-12  8:40   ` Tudor.Ambarus
  2022-07-18  7:21     ` Michael Walle
@ 2022-07-18  7:25     ` Michael Walle
  2022-07-19  6:00       ` Tudor.Ambarus
  1 sibling, 1 reply; 22+ messages in thread
From: Michael Walle @ 2022-07-18  7:25 UTC (permalink / raw)
  To: Tudor.Ambarus
  Cc: p.yadav, miquel.raynal, richard, vigneshr, quic_c_sbhanu,
	linux-mtd, linux-kernel

Am 2022-07-12 10:40, schrieb Tudor.Ambarus@microchip.com:
> Shaik, can we have your Tested-by tag on this?

Sigh. His email address bounces with "The email address you
entered couldn't be found." So don't expect a Tested-by: here.

-michael

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3()
  2022-05-10 14:02 ` [PATCH 1/2] mtd: spi-nor: " Michael Walle
  2022-06-05 15:00   ` Tom Fitzhenry
  2022-07-12  7:23   ` Pratyush Yadav
@ 2022-07-19  5:57   ` Tudor.Ambarus
  2022-07-19  7:07     ` Michael Walle
  2022-07-28  3:24     ` Tudor.Ambarus
  2 siblings, 2 replies; 22+ messages in thread
From: Tudor.Ambarus @ 2022-07-19  5:57 UTC (permalink / raw)
  To: michael, p.yadav, miquel.raynal, richard, vigneshr, quic_c_sbhanu
  Cc: linux-mtd, linux-kernel

On 5/10/22 17:02, Michael Walle wrote:

Hi!

> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Up until now, flashes were defined by specifying the JEDEC ID, the
> sector size and the number of sectors. This can be read by parsing the
> SFDP, we don't have to specify it. Thus provide a new macro SNOR_ID3()
> which just takes the JEDEC ID and implicitly set .parse_sfdp = true. All
> new flashes which have SFDP should use this macro.

I like the idea, but you need to refine it a bit.
Your assumptions are true only for flashes that are compliant with SFDP revB or
later because params->page_size is initialized by querying BFPT DWORD 11. I think
it would be good to specify this in the comment section. Also, I think you introduce
a bug in spi_nor_select_erase() when CONFIG_MTD_SPI_NOR_USE_4K_SECTORS is not
selected. wanted_size will be zero, will you select an invalid erase type?
Would you please resubmit?

Thanks,
ta

> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/core.c | 7 +++++--
>  drivers/mtd/spi-nor/core.h | 9 +++++++++
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 402b37cdbcea..29329ed0a934 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2104,8 +2104,11 @@ static int spi_nor_select_pp(struct spi_nor *nor,
>   * spi_nor_select_uniform_erase() - select optimum uniform erase type
>   * @map:               the erase map of the SPI NOR
>   * @wanted_size:       the erase type size to search for. Contains the value of
> - *                     info->sector_size or of the "small sector" size in case
> - *                     CONFIG_MTD_SPI_NOR_USE_4K_SECTORS is defined.
> + *                     info->sector_size, the "small sector" size in case
> + *                     CONFIG_MTD_SPI_NOR_USE_4K_SECTORS is defined or 0 if
> + *                     there is no information about the sector size. The
> + *                     latter is the case if the flash parameters are parsed
> + *                     solely by SFDP.
>   *
>   * Once the optimum uniform sector erase command is found, disable all the
>   * other.
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 61886868cd02..fab3038c4f4a 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -563,6 +563,15 @@ struct flash_info {
>                         .n_regions = (_n_regions),                      \
>                 },
> 
> +#define SNOR_ID3(_jedec_id)                                            \
> +               .id = {                                                 \
> +                       ((_jedec_id) >> 16) & 0xff,                     \
> +                       ((_jedec_id) >> 8) & 0xff,                      \
> +                       (_jedec_id) & 0xff,                             \
> +                       },                                              \
> +               .id_len = 3,                                            \
> +               .parse_sfdp = true,                                     \
> +
>  #define PARSE_SFDP                                                     \
>         .parse_sfdp = true,                                             \
> 
> --
> 2.30.2
> 

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/2] mtd: spi-nor: winbond: use SNOR_ID3() for w25q512nwm
  2022-07-18  7:25     ` Michael Walle
@ 2022-07-19  6:00       ` Tudor.Ambarus
  2022-07-19  7:02         ` Michael Walle
  0 siblings, 1 reply; 22+ messages in thread
From: Tudor.Ambarus @ 2022-07-19  6:00 UTC (permalink / raw)
  To: michael
  Cc: p.yadav, miquel.raynal, richard, vigneshr, quic_c_sbhanu,
	linux-mtd, linux-kernel

On 7/18/22 10:25, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2022-07-12 10:40, schrieb Tudor.Ambarus@microchip.com:
>> Shaik, can we have your Tested-by tag on this?
> 
> Sigh. His email address bounces with "The email address you
> entered couldn't be found." So don't expect a Tested-by: here.
> 

Would you drop this and pick the other patches that use your SNOR_ID3
and submit them all in a single patch set?
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/2] mtd: spi-nor: winbond: use SNOR_ID3() for w25q512nwm
  2022-07-19  6:00       ` Tudor.Ambarus
@ 2022-07-19  7:02         ` Michael Walle
  2022-07-19  7:24           ` Tudor.Ambarus
  0 siblings, 1 reply; 22+ messages in thread
From: Michael Walle @ 2022-07-19  7:02 UTC (permalink / raw)
  To: Tudor.Ambarus
  Cc: p.yadav, miquel.raynal, richard, vigneshr, linux-mtd, linux-kernel

Am 2022-07-19 08:00, schrieb Tudor.Ambarus@microchip.com:
> On 7/18/22 10:25, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> Am 2022-07-12 10:40, schrieb Tudor.Ambarus@microchip.com:
>>> Shaik, can we have your Tested-by tag on this?
>> 
>> Sigh. His email address bounces with "The email address you
>> entered couldn't be found." So don't expect a Tested-by: here.
>> 
> 
> Would you drop this and pick the other patches that use your SNOR_ID3
> and submit them all in a single patch set?

I can collect the other patches, but why drop this if we know
for a fact that the flash will work? (Because it is the same
as the w25q512nwq)

-michael

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3()
  2022-07-19  5:57   ` Tudor.Ambarus
@ 2022-07-19  7:07     ` Michael Walle
  2022-07-19  7:33       ` Tudor.Ambarus
  2022-07-28  3:24     ` Tudor.Ambarus
  1 sibling, 1 reply; 22+ messages in thread
From: Michael Walle @ 2022-07-19  7:07 UTC (permalink / raw)
  To: Tudor.Ambarus
  Cc: p.yadav, miquel.raynal, richard, vigneshr, linux-mtd, linux-kernel

Am 2022-07-19 07:57, schrieb Tudor.Ambarus@microchip.com:
> On 5/10/22 17:02, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> Up until now, flashes were defined by specifying the JEDEC ID, the
>> sector size and the number of sectors. This can be read by parsing the
>> SFDP, we don't have to specify it. Thus provide a new macro SNOR_ID3()
>> which just takes the JEDEC ID and implicitly set .parse_sfdp = true. 
>> All
>> new flashes which have SFDP should use this macro.
> 
> I like the idea, but you need to refine it a bit.
> Your assumptions are true only for flashes that are compliant with SFDP 
> revB or
> later because params->page_size is initialized by querying BFPT DWORD
> 11. I think it would be good to specify this in the comment section.

Sure.

> Also, I think you introduce
> a bug in spi_nor_select_erase() when CONFIG_MTD_SPI_NOR_USE_4K_SECTORS 
> is not
> selected. wanted_size will be zero, will you select an invalid erase 
> type?

You mean to squeeze [1] into this one? If so, sure.

-michael

[1] 
https://lore.kernel.org/linux-mtd/20220716000643.3541839-1-quic_jaehyoo@quicinc.com/

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/2] mtd: spi-nor: winbond: use SNOR_ID3() for w25q512nwm
  2022-07-19  7:02         ` Michael Walle
@ 2022-07-19  7:24           ` Tudor.Ambarus
  0 siblings, 0 replies; 22+ messages in thread
From: Tudor.Ambarus @ 2022-07-19  7:24 UTC (permalink / raw)
  To: michael
  Cc: p.yadav, miquel.raynal, richard, vigneshr, linux-mtd, linux-kernel

On 7/19/22 10:02, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2022-07-19 08:00, schrieb Tudor.Ambarus@microchip.com:
>> On 7/18/22 10:25, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> Am 2022-07-12 10:40, schrieb Tudor.Ambarus@microchip.com:
>>>> Shaik, can we have your Tested-by tag on this?
>>>
>>> Sigh. His email address bounces with "The email address you
>>> entered couldn't be found." So don't expect a Tested-by: here.
>>>
>>
>> Would you drop this and pick the other patches that use your SNOR_ID3
>> and submit them all in a single patch set?
> 
> I can collect the other patches, but why drop this if we know
> for a fact that the flash will work? (Because it is the same
> as the w25q512nwq)
> 

Was it tested? I don't mind to queue it, but I thought we only queue
changes that were tested on actual hw.

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3()
  2022-07-19  7:07     ` Michael Walle
@ 2022-07-19  7:33       ` Tudor.Ambarus
  2022-07-19  7:57         ` Michael Walle
  0 siblings, 1 reply; 22+ messages in thread
From: Tudor.Ambarus @ 2022-07-19  7:33 UTC (permalink / raw)
  To: michael
  Cc: p.yadav, miquel.raynal, richard, vigneshr, linux-mtd, linux-kernel

On 7/19/22 10:07, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2022-07-19 07:57, schrieb Tudor.Ambarus@microchip.com:
>> On 5/10/22 17:02, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> Up until now, flashes were defined by specifying the JEDEC ID, the
>>> sector size and the number of sectors. This can be read by parsing the
>>> SFDP, we don't have to specify it. Thus provide a new macro SNOR_ID3()
>>> which just takes the JEDEC ID and implicitly set .parse_sfdp = true.
>>> All
>>> new flashes which have SFDP should use this macro.
>>
>> I like the idea, but you need to refine it a bit.
>> Your assumptions are true only for flashes that are compliant with SFDP
>> revB or
>> later because params->page_size is initialized by querying BFPT DWORD
>> 11. I think it would be good to specify this in the comment section.
> 
> Sure.
> 
>> Also, I think you introduce
>> a bug in spi_nor_select_erase() when CONFIG_MTD_SPI_NOR_USE_4K_SECTORS
>> is not
>> selected. wanted_size will be zero, will you select an invalid erase
>> type?
> 
> You mean to squeeze [1] into this one? If so, sure.
> 
> -michael
> 
> [1]
> https://lore.kernel.org/linux-mtd/20220716000643.3541839-1-quic_jaehyoo@quicinc.com/

No, these are orthogonal. If you keep wanted_size to zero, then
spi_nor_select_uniform_erase() will return NULL, doesn't it?


Maybe to adapt the code to something like
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 21cefe2864ba..dd6cd852d1ef 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2148,7 +2148,7 @@ static int spi_nor_select_erase(struct spi_nor *nor)
        struct spi_nor_erase_map *map = &nor->params->erase_map;
        const struct spi_nor_erase_type *erase = NULL;
        struct mtd_info *mtd = &nor->mtd;
-       u32 wanted_size = nor->info->sector_size;
+       u32 wanted_size = nor->params->sector_size;

and fill nor->params->sector_size even when no SFDP

Also, params->size = (u64)info->sector_size * info->n_sectors; from
spi_nor_init_default_params() becomes superfluous. I would check
the fields that I don't initialize in flash_info with SNOR_ID3
and check how I can mitigate their absence throughout the code.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3()
  2022-07-19  7:33       ` Tudor.Ambarus
@ 2022-07-19  7:57         ` Michael Walle
  2022-07-19  8:30           ` Tudor.Ambarus
  0 siblings, 1 reply; 22+ messages in thread
From: Michael Walle @ 2022-07-19  7:57 UTC (permalink / raw)
  To: Tudor.Ambarus
  Cc: p.yadav, miquel.raynal, richard, vigneshr, linux-mtd, linux-kernel

Am 2022-07-19 09:33, schrieb Tudor.Ambarus@microchip.com:
> On 7/19/22 10:07, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> Am 2022-07-19 07:57, schrieb Tudor.Ambarus@microchip.com:
>>> On 5/10/22 17:02, Michael Walle wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you 
>>>> know
>>>> the content is safe
>>>> 
>>>> Up until now, flashes were defined by specifying the JEDEC ID, the
>>>> sector size and the number of sectors. This can be read by parsing 
>>>> the
>>>> SFDP, we don't have to specify it. Thus provide a new macro 
>>>> SNOR_ID3()
>>>> which just takes the JEDEC ID and implicitly set .parse_sfdp = true.
>>>> All
>>>> new flashes which have SFDP should use this macro.
>>> 
>>> I like the idea, but you need to refine it a bit.
>>> Your assumptions are true only for flashes that are compliant with 
>>> SFDP
>>> revB or
>>> later because params->page_size is initialized by querying BFPT DWORD
>>> 11. I think it would be good to specify this in the comment section.
>> 
>> Sure.
>> 
>>> Also, I think you introduce
>>> a bug in spi_nor_select_erase() when 
>>> CONFIG_MTD_SPI_NOR_USE_4K_SECTORS
>>> is not
>>> selected. wanted_size will be zero, will you select an invalid erase
>>> type?
>> 
>> You mean to squeeze [1] into this one? If so, sure.
>> 
>> -michael
>> 
>> [1]
>> https://lore.kernel.org/linux-mtd/20220716000643.3541839-1-quic_jaehyoo@quicinc.com/
> 
> No, these are orthogonal. If you keep wanted_size to zero, then
> spi_nor_select_uniform_erase() will return NULL, doesn't it?

No, have a look at the second condition

if (!erase && tested_erase->size)
   erase = ..

So it will return the first non-empty slot. Thus it will
only return NULL if all the slots are empty (given the
fix is included).

Actually, I'd have expected that to mask out an erase
type, you clear the corresponding bit in uniform_erase_type,
in which case the for loop in spi_nor_select_uniform_erase()
would have just worked. But apparently there are two differnt
mechanism here to mark an entry as unused, either the size
is zero or the bit is not set. But that is a topic for another
patch.

-michael

> Maybe to adapt the code to something like
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 21cefe2864ba..dd6cd852d1ef 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2148,7 +2148,7 @@ static int spi_nor_select_erase(struct spi_nor 
> *nor)
>         struct spi_nor_erase_map *map = &nor->params->erase_map;
>         const struct spi_nor_erase_type *erase = NULL;
>         struct mtd_info *mtd = &nor->mtd;
> -       u32 wanted_size = nor->info->sector_size;
> +       u32 wanted_size = nor->params->sector_size;
> 
> and fill nor->params->sector_size even when no SFDP
> 
> Also, params->size = (u64)info->sector_size * info->n_sectors; from
> spi_nor_init_default_params() becomes superfluous. I would check
> the fields that I don't initialize in flash_info with SNOR_ID3
> and check how I can mitigate their absence throughout the code.

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3()
  2022-07-19  7:57         ` Michael Walle
@ 2022-07-19  8:30           ` Tudor.Ambarus
  0 siblings, 0 replies; 22+ messages in thread
From: Tudor.Ambarus @ 2022-07-19  8:30 UTC (permalink / raw)
  To: michael
  Cc: p.yadav, miquel.raynal, richard, vigneshr, linux-mtd, linux-kernel

On 7/19/22 10:57, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2022-07-19 09:33, schrieb Tudor.Ambarus@microchip.com:
>> On 7/19/22 10:07, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> Am 2022-07-19 07:57, schrieb Tudor.Ambarus@microchip.com:
>>>> On 5/10/22 17:02, Michael Walle wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>>> know
>>>>> the content is safe
>>>>>
>>>>> Up until now, flashes were defined by specifying the JEDEC ID, the
>>>>> sector size and the number of sectors. This can be read by parsing
>>>>> the
>>>>> SFDP, we don't have to specify it. Thus provide a new macro
>>>>> SNOR_ID3()
>>>>> which just takes the JEDEC ID and implicitly set .parse_sfdp = true.
>>>>> All
>>>>> new flashes which have SFDP should use this macro.
>>>>
>>>> I like the idea, but you need to refine it a bit.
>>>> Your assumptions are true only for flashes that are compliant with
>>>> SFDP
>>>> revB or
>>>> later because params->page_size is initialized by querying BFPT DWORD
>>>> 11. I think it would be good to specify this in the comment section.
>>>
>>> Sure.
>>>
>>>> Also, I think you introduce
>>>> a bug in spi_nor_select_erase() when
>>>> CONFIG_MTD_SPI_NOR_USE_4K_SECTORS
>>>> is not
>>>> selected. wanted_size will be zero, will you select an invalid erase
>>>> type?
>>>
>>> You mean to squeeze [1] into this one? If so, sure.
>>>
>>> -michael
>>>
>>> [1]
>>> https://lore.kernel.org/linux-mtd/20220716000643.3541839-1-quic_jaehyoo@quicinc.com/
>>
>> No, these are orthogonal. If you keep wanted_size to zero, then
>> spi_nor_select_uniform_erase() will return NULL, doesn't it?
> 
> No, have a look at the second condition
> 
> if (!erase && tested_erase->size)
>   erase = ..
> 
> So it will return the first non-empty slot. Thus it will
> only return NULL if all the slots are empty (given the
> fix is included).
> 
> Actually, I'd have expected that to mask out an erase
> type, you clear the corresponding bit in uniform_erase_type,
> in which case the for loop in spi_nor_select_uniform_erase()
> would have just worked. But apparently there are two differnt
> mechanism here to mark an entry as unused, either the size
> is zero or the bit is not set. But that is a topic for another
> patch.


Right, I remember I leaned towards using just the erase mask to mask
out an erase, but I have to reassess this. Here's a patch that is
related and I left behind:
https://patchwork.ozlabs.org/project/linux-mtd/patch/20211119081412.29732-1-alexander.sverdlin@nokia.com/

> 

something else that looks wrong:
drivers/mtd/spi-nor/swp.c:              return nor->info->sector_size <<
drivers/mtd/spi-nor/swp.c:              return nor->info->sector_size;

How do we progress on this? I like the SNOR_ID3 idea, but I think it
should have a different form. Do you want to spend more time on this
or do you think I should invest more time on this?


> -michael
> 
>> Maybe to adapt the code to something like
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 21cefe2864ba..dd6cd852d1ef 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -2148,7 +2148,7 @@ static int spi_nor_select_erase(struct spi_nor
>> *nor)
>>         struct spi_nor_erase_map *map = &nor->params->erase_map;
>>         const struct spi_nor_erase_type *erase = NULL;
>>         struct mtd_info *mtd = &nor->mtd;
>> -       u32 wanted_size = nor->info->sector_size;
>> +       u32 wanted_size = nor->params->sector_size;
>>
>> and fill nor->params->sector_size even when no SFDP
>>
>> Also, params->size = (u64)info->sector_size * info->n_sectors; from
>> spi_nor_init_default_params() becomes superfluous. I would check
>> the fields that I don't initialize in flash_info with SNOR_ID3
>> and check how I can mitigate their absence throughout the code.

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3()
  2022-07-19  5:57   ` Tudor.Ambarus
  2022-07-19  7:07     ` Michael Walle
@ 2022-07-28  3:24     ` Tudor.Ambarus
  2022-07-28 13:12       ` Michael Walle
  1 sibling, 1 reply; 22+ messages in thread
From: Tudor.Ambarus @ 2022-07-28  3:24 UTC (permalink / raw)
  To: michael, p.yadav, miquel.raynal, richard, vigneshr, quic_c_sbhanu
  Cc: linux-mtd, linux-kernel

On 7/19/22 08:57, Tudor.Ambarus@microchip.com wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 5/10/22 17:02, Michael Walle wrote:
> 
> Hi!
> 
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> Up until now, flashes were defined by specifying the JEDEC ID, the
>> sector size and the number of sectors. This can be read by parsing the
>> SFDP, we don't have to specify it. Thus provide a new macro SNOR_ID3()
>> which just takes the JEDEC ID and implicitly set .parse_sfdp = true. All
>> new flashes which have SFDP should use this macro.
> 
> I like the idea, but you need to refine it a bit.
> Your assumptions are true only for flashes that are compliant with SFDP revB or
> later because params->page_size is initialized by querying BFPT DWORD 11. I think
> it would be good to specify this in the comment section. Also, I think you introduce
> a bug in spi_nor_select_erase() when CONFIG_MTD_SPI_NOR_USE_4K_SECTORS is not
> selected. wanted_size will be zero, will you select an invalid erase type?
> Would you please resubmit?
> 
> Thanks,
> ta
> 
>>
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>  drivers/mtd/spi-nor/core.c | 7 +++++--
>>  drivers/mtd/spi-nor/core.h | 9 +++++++++
>>  2 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 402b37cdbcea..29329ed0a934 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -2104,8 +2104,11 @@ static int spi_nor_select_pp(struct spi_nor *nor,
>>   * spi_nor_select_uniform_erase() - select optimum uniform erase type
>>   * @map:               the erase map of the SPI NOR
>>   * @wanted_size:       the erase type size to search for. Contains the value of
>> - *                     info->sector_size or of the "small sector" size in case
>> - *                     CONFIG_MTD_SPI_NOR_USE_4K_SECTORS is defined.
>> + *                     info->sector_size, the "small sector" size in case
>> + *                     CONFIG_MTD_SPI_NOR_USE_4K_SECTORS is defined or 0 if
>> + *                     there is no information about the sector size. The
>> + *                     latter is the case if the flash parameters are parsed
>> + *                     solely by SFDP.
>>   *
>>   * Once the optimum uniform sector erase command is found, disable all the
>>   * other.
>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>> index 61886868cd02..fab3038c4f4a 100644
>> --- a/drivers/mtd/spi-nor/core.h
>> +++ b/drivers/mtd/spi-nor/core.h
>> @@ -563,6 +563,15 @@ struct flash_info {
>>                         .n_regions = (_n_regions),                      \
>>                 },
>>
>> +#define SNOR_ID3(_jedec_id)

How about SFDP_ID3 and SFDP_ID6 instead?

                                            \
>> +               .id = {                                                 \
>> +                       ((_jedec_id) >> 16) & 0xff,                     \
>> +                       ((_jedec_id) >> 8) & 0xff,                      \
>> +                       (_jedec_id) & 0xff,                             \
>> +                       },                                              \
>> +               .id_len = 3,                                            \
>> +               .parse_sfdp = true,                                     \
>> +
>>  #define PARSE_SFDP                                                     \
>>         .parse_sfdp = true,                                             \
>>
>> --
>> 2.30.2
>>
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/


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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3()
  2022-07-28  3:24     ` Tudor.Ambarus
@ 2022-07-28 13:12       ` Michael Walle
  2022-07-28 13:31         ` Tudor.Ambarus
  0 siblings, 1 reply; 22+ messages in thread
From: Michael Walle @ 2022-07-28 13:12 UTC (permalink / raw)
  To: Tudor.Ambarus
  Cc: p.yadav, miquel.raynal, richard, vigneshr, quic_c_sbhanu,
	linux-mtd, linux-kernel

>>> +#define SNOR_ID3(_jedec_id)
> 
> How about SFDP_ID3 and SFDP_ID6 instead?

Yes, probably a better name. I was also thinking about splitting
the id in vendor, device and additional bytes. But I haven't
thought of the actual implementation that much. Such as:

#define SFDP_ID(<u8 vid>, <u16 did>, <variable aux bytes>)
#define SFDP_ID_FULL(<num_continuation_code>, <u8 vid>, <u16 did>, 
<variable aux bytes>)

Couldn't make up a better name than that _FULL for now. Happy to hear
suggestions :)

-michael

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3()
  2022-07-28 13:12       ` Michael Walle
@ 2022-07-28 13:31         ` Tudor.Ambarus
  2022-07-28 13:56           ` Michael Walle
  0 siblings, 1 reply; 22+ messages in thread
From: Tudor.Ambarus @ 2022-07-28 13:31 UTC (permalink / raw)
  To: michael
  Cc: p.yadav, miquel.raynal, richard, vigneshr, quic_c_sbhanu,
	linux-mtd, linux-kernel

On 7/28/22 16:12, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>>>> +#define SNOR_ID3(_jedec_id)
>>
>> How about SFDP_ID3 and SFDP_ID6 instead?
> 
> Yes, probably a better name. I was also thinking about splitting
> the id in vendor, device and additional bytes. But I haven't
> thought of the actual implementation that much. Such as:
> 
> #define SFDP_ID(<u8 vid>, <u16 did>, <variable aux bytes>)
> #define SFDP_ID_FULL(<num_continuation_code>, <u8 vid>, <u16 did>,
> <variable aux bytes>)
> 
> Couldn't make up a better name than that _FULL for now. Happy to hear
> suggestions :)
> 

You mean splitting the ID in manufacturer ID, flash ID and extended flash ID?
I'd like to understand the benefits of splitting this, can you give me an
example? In the past I though about introducing some flash info macros for
families of flashes of the same vendor, it will reduce the number of lines
on flash definition, but not really related.

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/2] mtd: spi-nor: introduce SNOR_ID3()
  2022-07-28 13:31         ` Tudor.Ambarus
@ 2022-07-28 13:56           ` Michael Walle
  0 siblings, 0 replies; 22+ messages in thread
From: Michael Walle @ 2022-07-28 13:56 UTC (permalink / raw)
  To: Tudor.Ambarus
  Cc: p.yadav, miquel.raynal, richard, vigneshr, quic_c_sbhanu,
	linux-mtd, linux-kernel

Am 2022-07-28 15:31, schrieb Tudor.Ambarus@microchip.com:
> On 7/28/22 16:12, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>>>>> +#define SNOR_ID3(_jedec_id)
>>> 
>>> How about SFDP_ID3 and SFDP_ID6 instead?
>> 
>> Yes, probably a better name. I was also thinking about splitting
>> the id in vendor, device and additional bytes. But I haven't
>> thought of the actual implementation that much. Such as:
>> 
>> #define SFDP_ID(<u8 vid>, <u16 did>, <variable aux bytes>)
>> #define SFDP_ID_FULL(<num_continuation_code>, <u8 vid>, <u16 did>,
>> <variable aux bytes>)
>> 
>> Couldn't make up a better name than that _FULL for now. Happy to hear
>> suggestions :)
>> 
> 
> You mean splitting the ID in manufacturer ID, flash ID and extended 
> flash ID?
> I'd like to understand the benefits of splitting this, can you give me 
> an
> example? In the past I though about introducing some flash info macros 
> for
> families of flashes of the same vendor, it will reduce the number of 
> lines
> on flash definition, but not really related.

First, why would you combine the vendor and part id into one three byte
field? Isn't it natural to have these as two fields? We know the did is
8 bit and the vid is 16 bit. And there might be N continuation codes. So
putting that all in one value is error prone. See also below for the
is25cd512.
Second, the extended (so maybe SFDP_ID_EXT?) bytes is variable. I've
seen flashes with one additional byte.

If you want to have per vendor convenience macros you could do

#define WINBOND_SFDP_ID(did, ...) SFDP_ID(0xNN, did, __VA_ARGS__)

I don't see much benefit here. OTHO "#define SFDP_VID_WINBOND 0xNN"
might make sense. But then we'd need to come up with support for
continuation codes. So mhhh.

As for the examples:
  SFDP_ID(SFDP_VID_WINBOND, 0x6019)
  SFDP_ID(SFDP_VID_MICRON, 0xba19, 0x10, 0x44, 0x00)
or maybe
  SFDP_ID(SFDP_VID_MICRON, 0xba19, 0x10, 0x44)

Currently we have the is25cd512 which (correctly) uses
continuation codes. So, we'd need have something like
#define SFDP_VID_ISSI_LEGACY 0x9d
#define SFDP_VID_ISSI 1, 0x9d

And preferrably we'd have something like:

  SFDP_ID(SFDP_VID_ISSI_LEGACY, 0x4013)
  SFDP_ID(SFDD_VID_ISSI, 0x20xx)

(note that "xx" is unknown here.. we are lacking that because
the entry is just using INFO() and I'm too lazy to look up the
datasheet now.)

Now I know that above probably won't compile, but maybe someone
could come up with macros which actually work :)

The following might work, but feels awful like a hack:
#define SFDP_VID_ISSI_LEGACY 0, 0x9d
#define SFDP_VID_ISSI 1, 0x9d

-michael

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2022-07-28 14:04 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-10 14:02 [PATCH 0/2] introduce SNOR_ID3() Michael Walle
2022-05-10 14:02 ` [PATCH 1/2] mtd: spi-nor: " Michael Walle
2022-06-05 15:00   ` Tom Fitzhenry
2022-07-12  7:23   ` Pratyush Yadav
2022-07-15 12:19     ` Biju Das
2022-07-19  5:57   ` Tudor.Ambarus
2022-07-19  7:07     ` Michael Walle
2022-07-19  7:33       ` Tudor.Ambarus
2022-07-19  7:57         ` Michael Walle
2022-07-19  8:30           ` Tudor.Ambarus
2022-07-28  3:24     ` Tudor.Ambarus
2022-07-28 13:12       ` Michael Walle
2022-07-28 13:31         ` Tudor.Ambarus
2022-07-28 13:56           ` Michael Walle
2022-05-10 14:02 ` [PATCH 2/2] mtd: spi-nor: winbond: use SNOR_ID3() for w25q512nwm Michael Walle
2022-07-12  8:40   ` Tudor.Ambarus
2022-07-18  7:21     ` Michael Walle
2022-07-18  7:25     ` Michael Walle
2022-07-19  6:00       ` Tudor.Ambarus
2022-07-19  7:02         ` Michael Walle
2022-07-19  7:24           ` Tudor.Ambarus
2022-05-10 14:03 ` [PATCH 0/2] mtd: spi-nor: introduce SNOR_ID3() Michael Walle

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).