diff mbox series

[v3,13/25] mtd: spi-nor: sst: Get rid of SST_WRITE flash_info flag

Message ID 20211029172633.886453-14-tudor.ambarus@microchip.com (mailing list archive)
State New, archived
Headers show
Series mtd: spi-nor: Clean params init | expand

Commit Message

Tudor Ambarus Oct. 29, 2021, 5:26 p.m. UTC 
The flash_info flags should be generic and not manufacturer specific.
Get rid of the manufacturer specific flag and use the late_init() fixup
hook instead.
Please note that sst_write is now set at flash level and not globally,
per manufacturer. Manufacturer hooks are generally a bad idea, because
it affects settings for all the flashes and we might end up with fixups
for "manufacturer settings".

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/core.h |   1 -
 drivers/mtd/spi-nor/sst.c  | 100 +++++++++++++++++++++----------------
 2 files changed, 58 insertions(+), 43 deletions(-)

Comments

Michael Walle Nov. 9, 2021, 12:21 p.m. UTC | #1 
Am 2021-10-29 19:26, schrieb Tudor Ambarus:
> The flash_info flags should be generic and not manufacturer specific.
> Get rid of the manufacturer specific flag and use the late_init() fixup
> hook instead.
> Please note that sst_write is now set at flash level and not globally,
> per manufacturer. Manufacturer hooks are generally a bad idea, because
> it affects settings for all the flashes and we might end up with fixups
> for "manufacturer settings".
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>

Reviewed-by: Michael Walle <michael@walle.cc>

I'm still not sure, if having a just one fixup function will scale with
different flashes. What do you think about having an additional (opaque
to the core) (bit)field for the manufacturer and flash fixups functions?
In this case, you can reuse the same function - and then a manufacturer
will make more sense (addressing Pratyush comment about a common
manufacturer fixup here).

I.e. the SST_WRITE flag would go into these flags, set per flash device
and the fixup can still remain in the manufacturer fixup.

-michael
Tudor Ambarus Nov. 9, 2021, 12:31 p.m. UTC | #2 
On 11/9/21 2:21 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2021-10-29 19:26, schrieb Tudor Ambarus:
>> The flash_info flags should be generic and not manufacturer specific.
>> Get rid of the manufacturer specific flag and use the late_init() fixup
>> hook instead.
>> Please note that sst_write is now set at flash level and not globally,
>> per manufacturer. Manufacturer hooks are generally a bad idea, because
>> it affects settings for all the flashes and we might end up with fixups
>> for "manufacturer settings".
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> Reviewed-by: Michael Walle <michael@walle.cc>
> 
> I'm still not sure, if having a just one fixup function will scale with
> different flashes. What do you think about having an additional (opaque
> to the core) (bit)field for the manufacturer and flash fixups functions?

Sounds fine. I can reserve few bits to manufacturers and introduce a 
MANUF_FLAGS genmask for the flash_info flags. Each specific manufacturer 
flag will be visible just for that manufacturer. Please check patch 14/25,
I can extend the idea from there.


> In this case, you can reuse the same function - and then a manufacturer
> will make more sense (addressing Pratyush comment about a common
> manufacturer fixup here).
> 
> I.e. the SST_WRITE flag would go into these flags, set per flash device
> and the fixup can still remain in the manufacturer fixup.
> 

Right. Let me know if a MANUF_FLAGS genmask is better than this patch.

Cheers,
ta
Michael Walle Nov. 12, 2021, 9:28 p.m. UTC | #3 
Am 2021-11-09 13:31, schrieb Tudor.Ambarus@microchip.com:
> On 11/9/21 2:21 PM, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> Am 2021-10-29 19:26, schrieb Tudor Ambarus:
>>> The flash_info flags should be generic and not manufacturer specific.
>>> Get rid of the manufacturer specific flag and use the late_init() 
>>> fixup
>>> hook instead.
>>> Please note that sst_write is now set at flash level and not 
>>> globally,
>>> per manufacturer. Manufacturer hooks are generally a bad idea, 
>>> because
>>> it affects settings for all the flashes and we might end up with 
>>> fixups
>>> for "manufacturer settings".
>>> 
>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> 
>> Reviewed-by: Michael Walle <michael@walle.cc>
>> 
>> I'm still not sure, if having a just one fixup function will scale 
>> with
>> different flashes. What do you think about having an additional 
>> (opaque
>> to the core) (bit)field for the manufacturer and flash fixups 
>> functions?
> 
> Sounds fine. I can reserve few bits to manufacturers and introduce a
> MANUF_FLAGS genmask for the flash_info flags. Each specific 
> manufacturer
> flag will be visible just for that manufacturer. Please check patch 
> 14/25,
> I can extend the idea from there.
> 
> 
>> In this case, you can reuse the same function - and then a 
>> manufacturer
>> will make more sense (addressing Pratyush comment about a common
>> manufacturer fixup here).
>> 
>> I.e. the SST_WRITE flag would go into these flags, set per flash 
>> device
>> and the fixup can still remain in the manufacturer fixup.
>> 
> 
> Right. Let me know if a MANUF_FLAGS genmask is better than this patch.

Yes, I think so.

-michael
diff mbox series

Patch

diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index f6c4b6f4743b..6fc63ef4267b 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -336,7 +336,6 @@  struct flash_info {
 	u32		flags;
 #define SECT_4K			BIT(0)	/* SPINOR_OP_BE_4K works uniformly */
 #define SPI_NOR_NO_ERASE	BIT(1)	/* No erase command needed */
-#define SST_WRITE		BIT(2)	/* use SST byte programming */
 #define SPI_NOR_NO_FR		BIT(3)	/* Can't do fastread */
 #define SECT_4K_PMC		BIT(4)	/* SPINOR_OP_BE_4K_PMC works uniformly */
 #define SPI_NOR_DUAL_READ	BIT(5)	/* Flash supports Dual Read */
diff --git a/drivers/mtd/spi-nor/sst.c b/drivers/mtd/spi-nor/sst.c
index 3593aae0920f..40e55b531edb 100644
--- a/drivers/mtd/spi-nor/sst.c
+++ b/drivers/mtd/spi-nor/sst.c
@@ -55,42 +55,6 @@  static const struct spi_nor_fixups sst26vf_fixups = {
 	.late_init = sst26vf_late_init,
 };
 
-static const struct flash_info sst_parts[] = {
-	/* SST -- large erase sizes are "overlays", "sectors" are 4K */
-	{ "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024,  8,
-			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_SWP_IS_VOLATILE) },
-	{ "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16,
-			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_SWP_IS_VOLATILE) },
-	{ "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32,
-			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_SWP_IS_VOLATILE) },
-	{ "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64,
-			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_SWP_IS_VOLATILE) },
-	{ "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128,
-			      SECT_4K | SPI_NOR_4BIT_BP | SPI_NOR_HAS_LOCK |
-			      SPI_NOR_SWP_IS_VOLATILE) },
-	{ "sst25wf512",  INFO(0xbf2501, 0, 64 * 1024,  1,
-			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_SWP_IS_VOLATILE) },
-	{ "sst25wf010",  INFO(0xbf2502, 0, 64 * 1024,  2,
-			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_SWP_IS_VOLATILE) },
-	{ "sst25wf020",  INFO(0xbf2503, 0, 64 * 1024,  4,
-			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_SWP_IS_VOLATILE) },
-	{ "sst25wf020a", INFO(0x621612, 0, 64 * 1024,  4, SECT_4K | SPI_NOR_HAS_LOCK) },
-	{ "sst25wf040b", INFO(0x621613, 0, 64 * 1024,  8, SECT_4K | SPI_NOR_HAS_LOCK) },
-	{ "sst25wf040",  INFO(0xbf2504, 0, 64 * 1024,  8,
-			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_SWP_IS_VOLATILE) },
-	{ "sst25wf080",  INFO(0xbf2505, 0, 64 * 1024, 16,
-			      SECT_4K | SST_WRITE | SPI_NOR_HAS_LOCK | SPI_NOR_SWP_IS_VOLATILE) },
-	{ "sst26wf016b", INFO(0xbf2651, 0, 64 * 1024, 32,
-			      SECT_4K | SPI_NOR_DUAL_READ |
-			      SPI_NOR_QUAD_READ) },
-	{ "sst26vf016b", INFO(0xbf2641, 0, 64 * 1024, 32,
-			      SECT_4K | SPI_NOR_DUAL_READ) },
-	{ "sst26vf064b", INFO(0xbf2643, 0, 64 * 1024, 128,
-			      SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
-			      SPI_NOR_HAS_LOCK | SPI_NOR_SWP_IS_VOLATILE)
-		.fixups = &sst26vf_fixups },
-};
-
 static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,
 		     size_t *retlen, const u_char *buf)
 {
@@ -177,19 +141,71 @@  static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,
 	return ret;
 }
 
-static void sst_late_init(struct spi_nor *nor)
+static void sst_write_late_init(struct spi_nor *nor)
 {
-	if (nor->info->flags & SST_WRITE)
-		nor->mtd._write = sst_write;
+	nor->mtd._write = sst_write;
 }
 
-static const struct spi_nor_fixups sst_fixups = {
-	.late_init = sst_late_init,
+static const struct spi_nor_fixups sst_write_fixup = {
+	.late_init = sst_write_late_init,
+};
+
+static const struct flash_info sst_parts[] = {
+	/* SST -- large erase sizes are "overlays", "sectors" are 4K */
+	{ "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024,  8,
+			      SECT_4K | SPI_NOR_HAS_LOCK |
+			      SPI_NOR_SWP_IS_VOLATILE)
+		.fixups = &sst_write_fixup },
+	{ "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16,
+			      SECT_4K | SPI_NOR_HAS_LOCK |
+			      SPI_NOR_SWP_IS_VOLATILE)
+		.fixups = &sst_write_fixup },
+	{ "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32,
+			      SECT_4K | SPI_NOR_HAS_LOCK |
+			      SPI_NOR_SWP_IS_VOLATILE)
+		.fixups = &sst_write_fixup },
+	{ "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64,
+			      SECT_4K | SPI_NOR_HAS_LOCK |
+			      SPI_NOR_SWP_IS_VOLATILE)
+		.fixups = &sst_write_fixup },
+	{ "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128,
+			      SECT_4K | SPI_NOR_4BIT_BP | SPI_NOR_HAS_LOCK |
+			      SPI_NOR_SWP_IS_VOLATILE) },
+	{ "sst25wf512",  INFO(0xbf2501, 0, 64 * 1024,  1,
+			      SECT_4K | SPI_NOR_HAS_LOCK |
+			      SPI_NOR_SWP_IS_VOLATILE)
+		.fixups = &sst_write_fixup },
+	{ "sst25wf010",  INFO(0xbf2502, 0, 64 * 1024,  2,
+			      SECT_4K | SPI_NOR_HAS_LOCK |
+			      SPI_NOR_SWP_IS_VOLATILE)
+		.fixups = &sst_write_fixup },
+	{ "sst25wf020",  INFO(0xbf2503, 0, 64 * 1024,  4,
+			      SECT_4K | SPI_NOR_HAS_LOCK |
+			      SPI_NOR_SWP_IS_VOLATILE)
+		.fixups = &sst_write_fixup },
+	{ "sst25wf020a", INFO(0x621612, 0, 64 * 1024,  4, SECT_4K | SPI_NOR_HAS_LOCK) },
+	{ "sst25wf040b", INFO(0x621613, 0, 64 * 1024,  8, SECT_4K | SPI_NOR_HAS_LOCK) },
+	{ "sst25wf040",  INFO(0xbf2504, 0, 64 * 1024,  8,
+			      SECT_4K | SPI_NOR_HAS_LOCK |
+			      SPI_NOR_SWP_IS_VOLATILE)
+		.fixups = &sst_write_fixup },
+	{ "sst25wf080",  INFO(0xbf2505, 0, 64 * 1024, 16,
+			      SECT_4K | SPI_NOR_HAS_LOCK |
+			      SPI_NOR_SWP_IS_VOLATILE)
+		.fixups = &sst_write_fixup },
+	{ "sst26wf016b", INFO(0xbf2651, 0, 64 * 1024, 32,
+			      SECT_4K | SPI_NOR_DUAL_READ |
+			      SPI_NOR_QUAD_READ) },
+	{ "sst26vf016b", INFO(0xbf2641, 0, 64 * 1024, 32,
+			      SECT_4K | SPI_NOR_DUAL_READ) },
+	{ "sst26vf064b", INFO(0xbf2643, 0, 64 * 1024, 128,
+			      SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
+			      SPI_NOR_HAS_LOCK | SPI_NOR_SWP_IS_VOLATILE)
+		.fixups = &sst26vf_fixups },
 };
 
 const struct spi_nor_manufacturer spi_nor_sst = {
 	.name = "sst",
 	.parts = sst_parts,
 	.nparts = ARRAY_SIZE(sst_parts),
-	.fixups = &sst_fixups,
 };