All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH RFC v1 1/2] mtd: spi-nor: export spi_nor_hwcaps_pp2cmd()
@ 2022-04-18 12:10 ` Michael Walle
  0 siblings, 0 replies; 10+ messages in thread
From: Michael Walle @ 2022-04-18 12:10 UTC (permalink / raw)
  To: Tudor Ambarus, Pratyush Yadav
  Cc: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	linux-mtd, linux-kernel, Michael Walle

The function will also be used by the debugfs module.

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

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index fd41ab9a8746..80d65cfcb88d 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1826,7 +1826,7 @@ int spi_nor_hwcaps_read2cmd(u32 hwcaps)
 				  ARRAY_SIZE(hwcaps_read2cmd));
 }
 
-static int spi_nor_hwcaps_pp2cmd(u32 hwcaps)
+int spi_nor_hwcaps_pp2cmd(u32 hwcaps)
 {
 	static const int hwcaps_pp2cmd[][2] = {
 		{ SNOR_HWCAPS_PP,		SNOR_CMD_PP },
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index beb622ae2779..81c4bb7d3193 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -553,6 +553,7 @@ int spi_nor_otp_lock_sr2(struct spi_nor *nor, unsigned int region);
 int spi_nor_otp_is_locked_sr2(struct spi_nor *nor, unsigned int region);
 
 int spi_nor_hwcaps_read2cmd(u32 hwcaps);
+int spi_nor_hwcaps_pp2cmd(u32 hwcaps);
 u8 spi_nor_convert_3to4_read(u8 opcode);
 void spi_nor_set_read_settings(struct spi_nor_read_command *read,
 			       u8 num_mode_clocks,
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH RFC v1 1/2] mtd: spi-nor: export spi_nor_hwcaps_pp2cmd()
@ 2022-04-18 12:10 ` Michael Walle
  0 siblings, 0 replies; 10+ messages in thread
From: Michael Walle @ 2022-04-18 12:10 UTC (permalink / raw)
  To: Tudor Ambarus, Pratyush Yadav
  Cc: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	linux-mtd, linux-kernel, Michael Walle

The function will also be used by the debugfs module.

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

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index fd41ab9a8746..80d65cfcb88d 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1826,7 +1826,7 @@ int spi_nor_hwcaps_read2cmd(u32 hwcaps)
 				  ARRAY_SIZE(hwcaps_read2cmd));
 }
 
-static int spi_nor_hwcaps_pp2cmd(u32 hwcaps)
+int spi_nor_hwcaps_pp2cmd(u32 hwcaps)
 {
 	static const int hwcaps_pp2cmd[][2] = {
 		{ SNOR_HWCAPS_PP,		SNOR_CMD_PP },
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index beb622ae2779..81c4bb7d3193 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -553,6 +553,7 @@ int spi_nor_otp_lock_sr2(struct spi_nor *nor, unsigned int region);
 int spi_nor_otp_is_locked_sr2(struct spi_nor *nor, unsigned int region);
 
 int spi_nor_hwcaps_read2cmd(u32 hwcaps);
+int spi_nor_hwcaps_pp2cmd(u32 hwcaps);
 u8 spi_nor_convert_3to4_read(u8 opcode);
 void spi_nor_set_read_settings(struct spi_nor_read_command *read,
 			       u8 num_mode_clocks,
-- 
2.30.2


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

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH RFC v1 2/2] mtd: spi-nor: expose internal parameters via debugfs
  2022-04-18 12:10 ` Michael Walle
@ 2022-04-18 12:10   ` Michael Walle
  -1 siblings, 0 replies; 10+ messages in thread
From: Michael Walle @ 2022-04-18 12:10 UTC (permalink / raw)
  To: Tudor Ambarus, Pratyush Yadav
  Cc: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	linux-mtd, linux-kernel, Michael Walle

There is no way to gather all information to verify support for a new
flash chip. Also if you want to convert an existing flash chip to the
new SFDP parsing, there is not enough information to determine if the
flash will work like before. To ease this development, expose internal
parameters via the debugfs.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/Makefile  |   1 +
 drivers/mtd/spi-nor/core.c    |   4 +
 drivers/mtd/spi-nor/core.h    |   8 ++
 drivers/mtd/spi-nor/debugfs.c | 241 ++++++++++++++++++++++++++++++++++
 include/linux/mtd/spi-nor.h   |   2 +
 5 files changed, 256 insertions(+)
 create mode 100644 drivers/mtd/spi-nor/debugfs.c

diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
index 6b904e439372..e347b435a038 100644
--- a/drivers/mtd/spi-nor/Makefile
+++ b/drivers/mtd/spi-nor/Makefile
@@ -17,6 +17,7 @@ spi-nor-objs			+= sst.o
 spi-nor-objs			+= winbond.o
 spi-nor-objs			+= xilinx.o
 spi-nor-objs			+= xmc.o
+spi-nor-$(CONFIG_DEBUG_FS)	+= debugfs.o
 obj-$(CONFIG_MTD_SPI_NOR)	+= spi-nor.o
 
 obj-$(CONFIG_MTD_SPI_NOR)	+= controllers/
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 80d65cfcb88d..302331695d96 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3117,6 +3117,8 @@ static int spi_nor_probe(struct spi_mem *spimem)
 	if (ret)
 		return ret;
 
+	spi_nor_debugfs_register(nor);
+
 	/*
 	 * None of the existing parts have > 512B pages, but let's play safe
 	 * and add this logic so that if anyone ever adds support for such
@@ -3148,6 +3150,8 @@ static int spi_nor_remove(struct spi_mem *spimem)
 {
 	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
 
+	spi_nor_debugfs_unregister(nor);
+
 	spi_nor_restore(nor);
 
 	/* Clean up MTD stuff. */
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 81c4bb7d3193..d042d745a1f6 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -589,4 +589,12 @@ static inline struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)
 	return container_of(mtd, struct spi_nor, mtd);
 }
 
+#ifdef CONFIG_DEBUG_FS
+void spi_nor_debugfs_register(struct spi_nor *nor);
+void spi_nor_debugfs_unregister(struct spi_nor *nor);
+#else
+static inline void spi_nor_debugfs_register(struct spi_nor *nor) {}
+static inline void spi_nor_debugfs_unregister(struct spi_nor *nor) {}
+#endif
+
 #endif /* __LINUX_MTD_SPI_NOR_INTERNAL_H */
diff --git a/drivers/mtd/spi-nor/debugfs.c b/drivers/mtd/spi-nor/debugfs.c
new file mode 100644
index 000000000000..61d6d90eda13
--- /dev/null
+++ b/drivers/mtd/spi-nor/debugfs.c
@@ -0,0 +1,241 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/mtd/spi-nor.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/spi-mem.h>
+#include <linux/debugfs.h>
+
+#include "core.h"
+
+static struct dentry *rootdir;
+
+#define SNOR_F_NAME(name) [ilog2(SNOR_F_##name)] = #name
+static const char *const snor_f_names[] = {
+	SNOR_F_NAME(HAS_SR_TB),
+	SNOR_F_NAME(NO_OP_CHIP_ERASE),
+	SNOR_F_NAME(BROKEN_RESET),
+	SNOR_F_NAME(4B_OPCODES),
+	SNOR_F_NAME(HAS_4BAIT),
+	SNOR_F_NAME(HAS_LOCK),
+	SNOR_F_NAME(HAS_16BIT_SR),
+	SNOR_F_NAME(NO_READ_CR),
+	SNOR_F_NAME(HAS_SR_TB_BIT6),
+	SNOR_F_NAME(HAS_4BIT_BP),
+	SNOR_F_NAME(HAS_SR_BP3_BIT6),
+	SNOR_F_NAME(IO_MODE_EN_VOLATILE),
+	SNOR_F_NAME(SOFT_RESET),
+	SNOR_F_NAME(SWP_IS_VOLATILE),
+};
+#undef SNOR_F_NAME
+
+static const char *spi_nor_protocol_name(enum spi_nor_protocol proto)
+{
+	switch (proto) {
+	case SNOR_PROTO_1_1_1:     return "1S-1S-1S";
+	case SNOR_PROTO_1_1_2:     return "1S-1S-2S";
+	case SNOR_PROTO_1_1_4:     return "1S-1S-4S";
+	case SNOR_PROTO_1_1_8:     return "1S-1S-8S";
+	case SNOR_PROTO_1_2_2:     return "1S-2S-2S";
+	case SNOR_PROTO_1_4_4:     return "1S-4S-4S";
+	case SNOR_PROTO_1_8_8:     return "1S-8S-8S";
+	case SNOR_PROTO_2_2_2:     return "2S-2S-2S";
+	case SNOR_PROTO_4_4_4:     return "4S-4S-4S";
+	case SNOR_PROTO_8_8_8:     return "8S-8S-8S";
+	case SNOR_PROTO_1_1_1_DTR: return "1D-1D-1D";
+	case SNOR_PROTO_1_2_2_DTR: return "1D-2D-2D";
+	case SNOR_PROTO_1_4_4_DTR: return "1D-4D-4D";
+	case SNOR_PROTO_1_8_8_DTR: return "1D-8D-8D";
+	case SNOR_PROTO_8_8_8_DTR: return "8D-8D-8D";
+	}
+
+	return "<unknown>";
+}
+
+static void spi_nor_print_flags(struct seq_file *s, unsigned long flags,
+				const char *const *names, int names_len)
+{
+	bool sep = false;
+	int i;
+
+	for (i = 0; i < sizeof(flags) * BITS_PER_BYTE; i++) {
+		if (!(flags & BIT(i)))
+			continue;
+		if (sep)
+			seq_puts(s, "|");
+		sep = true;
+		if (i < names_len && names[i])
+			seq_puts(s, names[i]);
+		else
+			seq_printf(s, "1<<%d", i);
+	}
+}
+
+static int spi_nor_params_show(struct seq_file *s, void *data)
+{
+	struct spi_nor *nor = s->private;
+	struct spi_nor_flash_parameter *params = nor->params;
+	struct spi_nor_erase_map *erase_map = &params->erase_map;
+	struct spi_nor_erase_region *region;
+	const struct flash_info *info = nor->info;
+	char buf[16], *str;
+	int i;
+
+	seq_printf(s, "name\t\t%s\n", info->name);
+	seq_printf(s, "id\t\t%*phN\n", info->id_len, nor->info->id);
+	string_get_size(params->size, 1, STRING_UNITS_2, buf, sizeof(buf));
+	seq_printf(s, "size\t\t%s\n", buf);
+	seq_printf(s, "write size\t%u\n", params->writesize);
+	seq_printf(s, "page size\t%u\n", params->page_size);
+	seq_printf(s, "address width\t%u\n", nor->addr_width);
+
+	seq_puts(s, "flags\t\t");
+	spi_nor_print_flags(s, nor->flags, snor_f_names, sizeof(snor_f_names));
+	seq_puts(s, "\n");
+
+	seq_puts(s, "\nopcodes\n");
+	seq_printf(s, " read\t\t%02x\n", nor->read_opcode);
+	seq_printf(s, "  dummy cycles\t%d\n", nor->read_dummy);
+	seq_printf(s, " erase\t\t%02x\n", nor->erase_opcode);
+	seq_printf(s, " program\t%02x\n", nor->program_opcode);
+
+	switch (nor->cmd_ext_type) {
+	case SPI_NOR_EXT_NONE:
+		str = "none";
+		break;
+	case SPI_NOR_EXT_REPEAT:
+		str = "repeat";
+		break;
+	case SPI_NOR_EXT_INVERT:
+		str = "invert";
+		break;
+	default:
+		str = "<unknown>";
+		break;
+	}
+	seq_printf(s, " 8D extension\t%s\n", str);
+
+	seq_puts(s, "\nprotocols\n");
+	seq_printf(s, " read\t\t%s\n",
+		   spi_nor_protocol_name(nor->read_proto));
+	seq_printf(s, " write\t\t%s\n",
+		   spi_nor_protocol_name(nor->write_proto));
+	seq_printf(s, " register\t%s\n",
+		   spi_nor_protocol_name(nor->reg_proto));
+
+	seq_puts(s, "\nerase commands\n");
+	for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++) {
+		struct spi_nor_erase_type *et = &erase_map->erase_type[i];
+
+		if (et->size) {
+			string_get_size(et->size, 1, STRING_UNITS_2, buf,
+					sizeof(buf));
+			seq_printf(s, " %02x (%s) [%d]\n", et->opcode, buf, i);
+		}
+	}
+
+	if (!(nor->flags & SNOR_F_NO_OP_CHIP_ERASE)) {
+		string_get_size(params->size, 1, STRING_UNITS_2, buf, sizeof(buf));
+		seq_printf(s, " %02x (%s)\n", SPINOR_OP_CHIP_ERASE, buf);
+	}
+
+	seq_puts(s, "\nsector map\n");
+	seq_puts(s, " region              | erase mask | flags\n");
+	seq_puts(s, " --------------------+------------+----------\n");
+	for (region = erase_map->regions;
+	     region;
+	     region = spi_nor_region_next(region)) {
+		u64 start = region->offset & ~SNOR_ERASE_FLAGS_MASK;
+		u64 flags = region->offset & SNOR_ERASE_FLAGS_MASK;
+		u64 end = start + region->size - 1;
+
+		seq_printf(s, " %08llx - %08llx |     [%c%c%c%c] | %s\n",
+			   start, end,
+			   flags & BIT(0) ? '0' : ' ',
+			   flags & BIT(1) ? '1' : ' ',
+			   flags & BIT(2) ? '2' : ' ',
+			   flags & BIT(3) ? '3' : ' ',
+			   flags & SNOR_OVERLAID_REGION ? "overlaid" : "");
+	}
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(spi_nor_params);
+
+static void spi_nor_print_read_cmd(struct seq_file *s, u32 cap,
+				   struct spi_nor_read_command *cmd)
+{
+	seq_printf(s, "%s%s\n", spi_nor_protocol_name(cmd->proto),
+		   cap == SNOR_HWCAPS_READ_FAST ? " (fast read)" : "");
+	seq_printf(s, "  opcode\t%02x\n", cmd->opcode);
+	seq_printf(s, "  mode cycles\t%02x\n", cmd->num_mode_clocks);
+	seq_printf(s, "  dummy cycles\t%02x\n", cmd->num_wait_states);
+}
+
+static void spi_nor_print_pp_cmd(struct seq_file *s,
+				 struct spi_nor_pp_command *cmd)
+{
+	seq_printf(s, "%s\n", spi_nor_protocol_name(cmd->proto));
+	seq_printf(s, "  opcode\t%02x\n", cmd->opcode);
+}
+
+static int spi_nor_capabilities_show(struct seq_file *s, void *data)
+{
+	struct spi_nor *nor = s->private;
+	struct spi_nor_flash_parameter *params = nor->params;
+	u32 hwcaps = params->hwcaps.mask;
+	int i, cmd;
+
+	seq_puts(s, "Supported read modes by the flash\n");
+	for (i = 0; i < sizeof(hwcaps) * BITS_PER_BYTE; i++) {
+		if (!(hwcaps & BIT(i)))
+			continue;
+
+		cmd = spi_nor_hwcaps_read2cmd(BIT(i));
+		if (cmd < 0)
+			continue;
+
+		spi_nor_print_read_cmd(s, BIT(i), &params->reads[cmd]);
+		hwcaps &= ~BIT(i);
+	}
+
+	seq_puts(s, "\nSupported page program modes by the flash\n");
+	for (i = 0; i < sizeof(hwcaps) * BITS_PER_BYTE; i++) {
+		if (!(hwcaps & BIT(i)))
+			continue;
+
+		cmd = spi_nor_hwcaps_pp2cmd(BIT(i));
+		if (cmd < 0)
+			continue;
+
+		spi_nor_print_pp_cmd(s, &params->page_programs[cmd]);
+		hwcaps &= ~BIT(i);
+	}
+
+	if (hwcaps)
+		seq_printf(s, "\nunknown hwcaps %x\n", hwcaps);
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(spi_nor_capabilities);
+
+void spi_nor_debugfs_register(struct spi_nor *nor)
+{
+	struct dentry *d;
+
+	/* Create rootdir once. Will never be deleted again. */
+	if (!rootdir)
+		rootdir = debugfs_create_dir("spi-nor", NULL);
+
+	d = debugfs_create_dir(dev_name(nor->dev), rootdir);
+	nor->debugfs_root = d;
+
+	debugfs_create_file("params", 0444, d, nor, &spi_nor_params_fops);
+	debugfs_create_file("capabilities", 0444, d, nor,
+			    &spi_nor_capabilities_fops);
+}
+
+void spi_nor_debugfs_unregister(struct spi_nor *nor)
+{
+	debugfs_remove(nor->debugfs_root);
+	nor->debugfs_root = NULL;
+}
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 5e25a7b75ae2..7d43447768ee 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -365,6 +365,7 @@ struct spi_nor_flash_parameter;
  * @write_proto:	the SPI protocol for write operations
  * @reg_proto:		the SPI protocol for read_reg/write_reg/erase operations
  * @sfdp:		the SFDP data of the flash
+ * @debugfs_root:	pointer to the debugfs directory
  * @controller_ops:	SPI NOR controller driver specific operations.
  * @params:		[FLASH-SPECIFIC] SPI NOR flash parameters and settings.
  *                      The structure includes legacy flash parameters and
@@ -394,6 +395,7 @@ struct spi_nor {
 	u32			flags;
 	enum spi_nor_cmd_ext	cmd_ext_type;
 	struct sfdp		*sfdp;
+	struct dentry		*debugfs_root;
 
 	const struct spi_nor_controller_ops *controller_ops;
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH RFC v1 2/2] mtd: spi-nor: expose internal parameters via debugfs
@ 2022-04-18 12:10   ` Michael Walle
  0 siblings, 0 replies; 10+ messages in thread
From: Michael Walle @ 2022-04-18 12:10 UTC (permalink / raw)
  To: Tudor Ambarus, Pratyush Yadav
  Cc: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	linux-mtd, linux-kernel, Michael Walle

There is no way to gather all information to verify support for a new
flash chip. Also if you want to convert an existing flash chip to the
new SFDP parsing, there is not enough information to determine if the
flash will work like before. To ease this development, expose internal
parameters via the debugfs.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/Makefile  |   1 +
 drivers/mtd/spi-nor/core.c    |   4 +
 drivers/mtd/spi-nor/core.h    |   8 ++
 drivers/mtd/spi-nor/debugfs.c | 241 ++++++++++++++++++++++++++++++++++
 include/linux/mtd/spi-nor.h   |   2 +
 5 files changed, 256 insertions(+)
 create mode 100644 drivers/mtd/spi-nor/debugfs.c

diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
index 6b904e439372..e347b435a038 100644
--- a/drivers/mtd/spi-nor/Makefile
+++ b/drivers/mtd/spi-nor/Makefile
@@ -17,6 +17,7 @@ spi-nor-objs			+= sst.o
 spi-nor-objs			+= winbond.o
 spi-nor-objs			+= xilinx.o
 spi-nor-objs			+= xmc.o
+spi-nor-$(CONFIG_DEBUG_FS)	+= debugfs.o
 obj-$(CONFIG_MTD_SPI_NOR)	+= spi-nor.o
 
 obj-$(CONFIG_MTD_SPI_NOR)	+= controllers/
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 80d65cfcb88d..302331695d96 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3117,6 +3117,8 @@ static int spi_nor_probe(struct spi_mem *spimem)
 	if (ret)
 		return ret;
 
+	spi_nor_debugfs_register(nor);
+
 	/*
 	 * None of the existing parts have > 512B pages, but let's play safe
 	 * and add this logic so that if anyone ever adds support for such
@@ -3148,6 +3150,8 @@ static int spi_nor_remove(struct spi_mem *spimem)
 {
 	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
 
+	spi_nor_debugfs_unregister(nor);
+
 	spi_nor_restore(nor);
 
 	/* Clean up MTD stuff. */
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 81c4bb7d3193..d042d745a1f6 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -589,4 +589,12 @@ static inline struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)
 	return container_of(mtd, struct spi_nor, mtd);
 }
 
+#ifdef CONFIG_DEBUG_FS
+void spi_nor_debugfs_register(struct spi_nor *nor);
+void spi_nor_debugfs_unregister(struct spi_nor *nor);
+#else
+static inline void spi_nor_debugfs_register(struct spi_nor *nor) {}
+static inline void spi_nor_debugfs_unregister(struct spi_nor *nor) {}
+#endif
+
 #endif /* __LINUX_MTD_SPI_NOR_INTERNAL_H */
diff --git a/drivers/mtd/spi-nor/debugfs.c b/drivers/mtd/spi-nor/debugfs.c
new file mode 100644
index 000000000000..61d6d90eda13
--- /dev/null
+++ b/drivers/mtd/spi-nor/debugfs.c
@@ -0,0 +1,241 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/mtd/spi-nor.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/spi-mem.h>
+#include <linux/debugfs.h>
+
+#include "core.h"
+
+static struct dentry *rootdir;
+
+#define SNOR_F_NAME(name) [ilog2(SNOR_F_##name)] = #name
+static const char *const snor_f_names[] = {
+	SNOR_F_NAME(HAS_SR_TB),
+	SNOR_F_NAME(NO_OP_CHIP_ERASE),
+	SNOR_F_NAME(BROKEN_RESET),
+	SNOR_F_NAME(4B_OPCODES),
+	SNOR_F_NAME(HAS_4BAIT),
+	SNOR_F_NAME(HAS_LOCK),
+	SNOR_F_NAME(HAS_16BIT_SR),
+	SNOR_F_NAME(NO_READ_CR),
+	SNOR_F_NAME(HAS_SR_TB_BIT6),
+	SNOR_F_NAME(HAS_4BIT_BP),
+	SNOR_F_NAME(HAS_SR_BP3_BIT6),
+	SNOR_F_NAME(IO_MODE_EN_VOLATILE),
+	SNOR_F_NAME(SOFT_RESET),
+	SNOR_F_NAME(SWP_IS_VOLATILE),
+};
+#undef SNOR_F_NAME
+
+static const char *spi_nor_protocol_name(enum spi_nor_protocol proto)
+{
+	switch (proto) {
+	case SNOR_PROTO_1_1_1:     return "1S-1S-1S";
+	case SNOR_PROTO_1_1_2:     return "1S-1S-2S";
+	case SNOR_PROTO_1_1_4:     return "1S-1S-4S";
+	case SNOR_PROTO_1_1_8:     return "1S-1S-8S";
+	case SNOR_PROTO_1_2_2:     return "1S-2S-2S";
+	case SNOR_PROTO_1_4_4:     return "1S-4S-4S";
+	case SNOR_PROTO_1_8_8:     return "1S-8S-8S";
+	case SNOR_PROTO_2_2_2:     return "2S-2S-2S";
+	case SNOR_PROTO_4_4_4:     return "4S-4S-4S";
+	case SNOR_PROTO_8_8_8:     return "8S-8S-8S";
+	case SNOR_PROTO_1_1_1_DTR: return "1D-1D-1D";
+	case SNOR_PROTO_1_2_2_DTR: return "1D-2D-2D";
+	case SNOR_PROTO_1_4_4_DTR: return "1D-4D-4D";
+	case SNOR_PROTO_1_8_8_DTR: return "1D-8D-8D";
+	case SNOR_PROTO_8_8_8_DTR: return "8D-8D-8D";
+	}
+
+	return "<unknown>";
+}
+
+static void spi_nor_print_flags(struct seq_file *s, unsigned long flags,
+				const char *const *names, int names_len)
+{
+	bool sep = false;
+	int i;
+
+	for (i = 0; i < sizeof(flags) * BITS_PER_BYTE; i++) {
+		if (!(flags & BIT(i)))
+			continue;
+		if (sep)
+			seq_puts(s, "|");
+		sep = true;
+		if (i < names_len && names[i])
+			seq_puts(s, names[i]);
+		else
+			seq_printf(s, "1<<%d", i);
+	}
+}
+
+static int spi_nor_params_show(struct seq_file *s, void *data)
+{
+	struct spi_nor *nor = s->private;
+	struct spi_nor_flash_parameter *params = nor->params;
+	struct spi_nor_erase_map *erase_map = &params->erase_map;
+	struct spi_nor_erase_region *region;
+	const struct flash_info *info = nor->info;
+	char buf[16], *str;
+	int i;
+
+	seq_printf(s, "name\t\t%s\n", info->name);
+	seq_printf(s, "id\t\t%*phN\n", info->id_len, nor->info->id);
+	string_get_size(params->size, 1, STRING_UNITS_2, buf, sizeof(buf));
+	seq_printf(s, "size\t\t%s\n", buf);
+	seq_printf(s, "write size\t%u\n", params->writesize);
+	seq_printf(s, "page size\t%u\n", params->page_size);
+	seq_printf(s, "address width\t%u\n", nor->addr_width);
+
+	seq_puts(s, "flags\t\t");
+	spi_nor_print_flags(s, nor->flags, snor_f_names, sizeof(snor_f_names));
+	seq_puts(s, "\n");
+
+	seq_puts(s, "\nopcodes\n");
+	seq_printf(s, " read\t\t%02x\n", nor->read_opcode);
+	seq_printf(s, "  dummy cycles\t%d\n", nor->read_dummy);
+	seq_printf(s, " erase\t\t%02x\n", nor->erase_opcode);
+	seq_printf(s, " program\t%02x\n", nor->program_opcode);
+
+	switch (nor->cmd_ext_type) {
+	case SPI_NOR_EXT_NONE:
+		str = "none";
+		break;
+	case SPI_NOR_EXT_REPEAT:
+		str = "repeat";
+		break;
+	case SPI_NOR_EXT_INVERT:
+		str = "invert";
+		break;
+	default:
+		str = "<unknown>";
+		break;
+	}
+	seq_printf(s, " 8D extension\t%s\n", str);
+
+	seq_puts(s, "\nprotocols\n");
+	seq_printf(s, " read\t\t%s\n",
+		   spi_nor_protocol_name(nor->read_proto));
+	seq_printf(s, " write\t\t%s\n",
+		   spi_nor_protocol_name(nor->write_proto));
+	seq_printf(s, " register\t%s\n",
+		   spi_nor_protocol_name(nor->reg_proto));
+
+	seq_puts(s, "\nerase commands\n");
+	for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++) {
+		struct spi_nor_erase_type *et = &erase_map->erase_type[i];
+
+		if (et->size) {
+			string_get_size(et->size, 1, STRING_UNITS_2, buf,
+					sizeof(buf));
+			seq_printf(s, " %02x (%s) [%d]\n", et->opcode, buf, i);
+		}
+	}
+
+	if (!(nor->flags & SNOR_F_NO_OP_CHIP_ERASE)) {
+		string_get_size(params->size, 1, STRING_UNITS_2, buf, sizeof(buf));
+		seq_printf(s, " %02x (%s)\n", SPINOR_OP_CHIP_ERASE, buf);
+	}
+
+	seq_puts(s, "\nsector map\n");
+	seq_puts(s, " region              | erase mask | flags\n");
+	seq_puts(s, " --------------------+------------+----------\n");
+	for (region = erase_map->regions;
+	     region;
+	     region = spi_nor_region_next(region)) {
+		u64 start = region->offset & ~SNOR_ERASE_FLAGS_MASK;
+		u64 flags = region->offset & SNOR_ERASE_FLAGS_MASK;
+		u64 end = start + region->size - 1;
+
+		seq_printf(s, " %08llx - %08llx |     [%c%c%c%c] | %s\n",
+			   start, end,
+			   flags & BIT(0) ? '0' : ' ',
+			   flags & BIT(1) ? '1' : ' ',
+			   flags & BIT(2) ? '2' : ' ',
+			   flags & BIT(3) ? '3' : ' ',
+			   flags & SNOR_OVERLAID_REGION ? "overlaid" : "");
+	}
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(spi_nor_params);
+
+static void spi_nor_print_read_cmd(struct seq_file *s, u32 cap,
+				   struct spi_nor_read_command *cmd)
+{
+	seq_printf(s, "%s%s\n", spi_nor_protocol_name(cmd->proto),
+		   cap == SNOR_HWCAPS_READ_FAST ? " (fast read)" : "");
+	seq_printf(s, "  opcode\t%02x\n", cmd->opcode);
+	seq_printf(s, "  mode cycles\t%02x\n", cmd->num_mode_clocks);
+	seq_printf(s, "  dummy cycles\t%02x\n", cmd->num_wait_states);
+}
+
+static void spi_nor_print_pp_cmd(struct seq_file *s,
+				 struct spi_nor_pp_command *cmd)
+{
+	seq_printf(s, "%s\n", spi_nor_protocol_name(cmd->proto));
+	seq_printf(s, "  opcode\t%02x\n", cmd->opcode);
+}
+
+static int spi_nor_capabilities_show(struct seq_file *s, void *data)
+{
+	struct spi_nor *nor = s->private;
+	struct spi_nor_flash_parameter *params = nor->params;
+	u32 hwcaps = params->hwcaps.mask;
+	int i, cmd;
+
+	seq_puts(s, "Supported read modes by the flash\n");
+	for (i = 0; i < sizeof(hwcaps) * BITS_PER_BYTE; i++) {
+		if (!(hwcaps & BIT(i)))
+			continue;
+
+		cmd = spi_nor_hwcaps_read2cmd(BIT(i));
+		if (cmd < 0)
+			continue;
+
+		spi_nor_print_read_cmd(s, BIT(i), &params->reads[cmd]);
+		hwcaps &= ~BIT(i);
+	}
+
+	seq_puts(s, "\nSupported page program modes by the flash\n");
+	for (i = 0; i < sizeof(hwcaps) * BITS_PER_BYTE; i++) {
+		if (!(hwcaps & BIT(i)))
+			continue;
+
+		cmd = spi_nor_hwcaps_pp2cmd(BIT(i));
+		if (cmd < 0)
+			continue;
+
+		spi_nor_print_pp_cmd(s, &params->page_programs[cmd]);
+		hwcaps &= ~BIT(i);
+	}
+
+	if (hwcaps)
+		seq_printf(s, "\nunknown hwcaps %x\n", hwcaps);
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(spi_nor_capabilities);
+
+void spi_nor_debugfs_register(struct spi_nor *nor)
+{
+	struct dentry *d;
+
+	/* Create rootdir once. Will never be deleted again. */
+	if (!rootdir)
+		rootdir = debugfs_create_dir("spi-nor", NULL);
+
+	d = debugfs_create_dir(dev_name(nor->dev), rootdir);
+	nor->debugfs_root = d;
+
+	debugfs_create_file("params", 0444, d, nor, &spi_nor_params_fops);
+	debugfs_create_file("capabilities", 0444, d, nor,
+			    &spi_nor_capabilities_fops);
+}
+
+void spi_nor_debugfs_unregister(struct spi_nor *nor)
+{
+	debugfs_remove(nor->debugfs_root);
+	nor->debugfs_root = NULL;
+}
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 5e25a7b75ae2..7d43447768ee 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -365,6 +365,7 @@ struct spi_nor_flash_parameter;
  * @write_proto:	the SPI protocol for write operations
  * @reg_proto:		the SPI protocol for read_reg/write_reg/erase operations
  * @sfdp:		the SFDP data of the flash
+ * @debugfs_root:	pointer to the debugfs directory
  * @controller_ops:	SPI NOR controller driver specific operations.
  * @params:		[FLASH-SPECIFIC] SPI NOR flash parameters and settings.
  *                      The structure includes legacy flash parameters and
@@ -394,6 +395,7 @@ struct spi_nor {
 	u32			flags;
 	enum spi_nor_cmd_ext	cmd_ext_type;
 	struct sfdp		*sfdp;
+	struct dentry		*debugfs_root;
 
 	const struct spi_nor_controller_ops *controller_ops;
 
-- 
2.30.2


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

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH RFC v1 2/2] mtd: spi-nor: expose internal parameters via debugfs
  2022-04-18 12:10   ` Michael Walle
@ 2022-04-18 12:13     ` Michael Walle
  -1 siblings, 0 replies; 10+ messages in thread
From: Michael Walle @ 2022-04-18 12:13 UTC (permalink / raw)
  To: Tudor Ambarus, Pratyush Yadav
  Cc: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	linux-mtd, linux-kernel

Am 2022-04-18 14:10, schrieb Michael Walle:
> There is no way to gather all information to verify support for a new
> flash chip. Also if you want to convert an existing flash chip to the
> new SFDP parsing, there is not enough information to determine if the
> flash will work like before. To ease this development, expose internal
> parameters via the debugfs.

Here's an example:
# cat params
name		w25q32dw
id		ef6016
size		4.00 MiB
write size	1
page size	256
address width	3
flags		HAS_SR_TB|HAS_LOCK|HAS_16BIT_SR|NO_READ_CR|SOFT_RESET

opcodes
  read		3b
   dummy cycles	8
  erase		20
  program	02
  8D extension	none

protocols
  read		1S-1S-2S
  write		1S-1S-1S
  register	1S-1S-1S

erase commands
  20 (4.00 KiB) [1]
  52 (32.0 KiB) [2]
  d8 (64.0 KiB) [3]
  c7 (4.00 MiB)

sector map
  region              | erase mask | flags
  --------------------+------------+----------
  00000000 - 003fffff |     [ 123] |

# cat capabilities
Supported read modes by the flash
1S-1S-1S
   opcode	03
   mode cycles	00
   dummy cycles	00
1S-1S-1S (fast read)
   opcode	0b
   mode cycles	00
   dummy cycles	08
1S-1S-2S
   opcode	3b
   mode cycles	00
   dummy cycles	08
1S-2S-2S
   opcode	bb
   mode cycles	02
   dummy cycles	02
1S-1S-4S
   opcode	6b
   mode cycles	00
   dummy cycles	08
1S-4S-4S
   opcode	eb
   mode cycles	02
   dummy cycles	04
4S-4S-4S
   opcode	eb
   mode cycles	02
   dummy cycles	00

Supported page program modes by the flash
1S-1S-1S
   opcode	02

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH RFC v1 2/2] mtd: spi-nor: expose internal parameters via debugfs
@ 2022-04-18 12:13     ` Michael Walle
  0 siblings, 0 replies; 10+ messages in thread
From: Michael Walle @ 2022-04-18 12:13 UTC (permalink / raw)
  To: Tudor Ambarus, Pratyush Yadav
  Cc: Miquel Raynal, Richard Weinberger, Vignesh Raghavendra,
	linux-mtd, linux-kernel

Am 2022-04-18 14:10, schrieb Michael Walle:
> There is no way to gather all information to verify support for a new
> flash chip. Also if you want to convert an existing flash chip to the
> new SFDP parsing, there is not enough information to determine if the
> flash will work like before. To ease this development, expose internal
> parameters via the debugfs.

Here's an example:
# cat params
name		w25q32dw
id		ef6016
size		4.00 MiB
write size	1
page size	256
address width	3
flags		HAS_SR_TB|HAS_LOCK|HAS_16BIT_SR|NO_READ_CR|SOFT_RESET

opcodes
  read		3b
   dummy cycles	8
  erase		20
  program	02
  8D extension	none

protocols
  read		1S-1S-2S
  write		1S-1S-1S
  register	1S-1S-1S

erase commands
  20 (4.00 KiB) [1]
  52 (32.0 KiB) [2]
  d8 (64.0 KiB) [3]
  c7 (4.00 MiB)

sector map
  region              | erase mask | flags
  --------------------+------------+----------
  00000000 - 003fffff |     [ 123] |

# cat capabilities
Supported read modes by the flash
1S-1S-1S
   opcode	03
   mode cycles	00
   dummy cycles	00
1S-1S-1S (fast read)
   opcode	0b
   mode cycles	00
   dummy cycles	08
1S-1S-2S
   opcode	3b
   mode cycles	00
   dummy cycles	08
1S-2S-2S
   opcode	bb
   mode cycles	02
   dummy cycles	02
1S-1S-4S
   opcode	6b
   mode cycles	00
   dummy cycles	08
1S-4S-4S
   opcode	eb
   mode cycles	02
   dummy cycles	04
4S-4S-4S
   opcode	eb
   mode cycles	02
   dummy cycles	00

Supported page program modes by the flash
1S-1S-1S
   opcode	02

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH RFC v1 2/2] mtd: spi-nor: expose internal parameters via debugfs
  2022-04-18 12:10   ` Michael Walle
@ 2022-04-20  5:23     ` Pratyush Yadav
  -1 siblings, 0 replies; 10+ messages in thread
From: Pratyush Yadav @ 2022-04-20  5:23 UTC (permalink / raw)
  To: Michael Walle
  Cc: Tudor Ambarus, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, linux-mtd, linux-kernel

Hi Michael,

On 18/04/22 02:10PM, Michael Walle wrote:
> There is no way to gather all information to verify support for a new
> flash chip. Also if you want to convert an existing flash chip to the
> new SFDP parsing, there is not enough information to determine if the
> flash will work like before. To ease this development, expose internal
> parameters via the debugfs.

A big +1 for this patch from me. I have too often added prints in the 
driver to find out all this information. This won't be very useful in 
case the probe fails, but I don't suppose we can do much about that.

> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/Makefile  |   1 +
>  drivers/mtd/spi-nor/core.c    |   4 +
>  drivers/mtd/spi-nor/core.h    |   8 ++
>  drivers/mtd/spi-nor/debugfs.c | 241 ++++++++++++++++++++++++++++++++++
>  include/linux/mtd/spi-nor.h   |   2 +
>  5 files changed, 256 insertions(+)
>  create mode 100644 drivers/mtd/spi-nor/debugfs.c
> 
> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
> index 6b904e439372..e347b435a038 100644
> --- a/drivers/mtd/spi-nor/Makefile
> +++ b/drivers/mtd/spi-nor/Makefile
> @@ -17,6 +17,7 @@ spi-nor-objs			+= sst.o
>  spi-nor-objs			+= winbond.o
>  spi-nor-objs			+= xilinx.o
>  spi-nor-objs			+= xmc.o
> +spi-nor-$(CONFIG_DEBUG_FS)	+= debugfs.o
>  obj-$(CONFIG_MTD_SPI_NOR)	+= spi-nor.o
>  
>  obj-$(CONFIG_MTD_SPI_NOR)	+= controllers/
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 80d65cfcb88d..302331695d96 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3117,6 +3117,8 @@ static int spi_nor_probe(struct spi_mem *spimem)
>  	if (ret)
>  		return ret;
>  
> +	spi_nor_debugfs_register(nor);

I think you should register this after mtd_device_register() to be sure 
the probe would not fail with these debugfs entries still in place. Or 
clean up before returning errors in the code below. Otherwise access to 
the debugfs entires would cause you to access a nor struct that is no 
longer there.

> +
>  	/*
>  	 * None of the existing parts have > 512B pages, but let's play safe
>  	 * and add this logic so that if anyone ever adds support for such
> @@ -3148,6 +3150,8 @@ static int spi_nor_remove(struct spi_mem *spimem)
>  {
>  	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
>  
> +	spi_nor_debugfs_unregister(nor);
> +
>  	spi_nor_restore(nor);
>  
>  	/* Clean up MTD stuff. */
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 81c4bb7d3193..d042d745a1f6 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -589,4 +589,12 @@ static inline struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)
>  	return container_of(mtd, struct spi_nor, mtd);
>  }
>  
> +#ifdef CONFIG_DEBUG_FS
> +void spi_nor_debugfs_register(struct spi_nor *nor);
> +void spi_nor_debugfs_unregister(struct spi_nor *nor);
> +#else
> +static inline void spi_nor_debugfs_register(struct spi_nor *nor) {}
> +static inline void spi_nor_debugfs_unregister(struct spi_nor *nor) {}
> +#endif
> +
>  #endif /* __LINUX_MTD_SPI_NOR_INTERNAL_H */
> diff --git a/drivers/mtd/spi-nor/debugfs.c b/drivers/mtd/spi-nor/debugfs.c
> new file mode 100644
> index 000000000000..61d6d90eda13
> --- /dev/null
> +++ b/drivers/mtd/spi-nor/debugfs.c
> @@ -0,0 +1,241 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/mtd/spi-nor.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/spi-mem.h>
> +#include <linux/debugfs.h>
> +
> +#include "core.h"
> +
> +static struct dentry *rootdir;
> +
> +#define SNOR_F_NAME(name) [ilog2(SNOR_F_##name)] = #name
> +static const char *const snor_f_names[] = {
> +	SNOR_F_NAME(HAS_SR_TB),
> +	SNOR_F_NAME(NO_OP_CHIP_ERASE),
> +	SNOR_F_NAME(BROKEN_RESET),
> +	SNOR_F_NAME(4B_OPCODES),
> +	SNOR_F_NAME(HAS_4BAIT),
> +	SNOR_F_NAME(HAS_LOCK),
> +	SNOR_F_NAME(HAS_16BIT_SR),
> +	SNOR_F_NAME(NO_READ_CR),
> +	SNOR_F_NAME(HAS_SR_TB_BIT6),
> +	SNOR_F_NAME(HAS_4BIT_BP),
> +	SNOR_F_NAME(HAS_SR_BP3_BIT6),
> +	SNOR_F_NAME(IO_MODE_EN_VOLATILE),
> +	SNOR_F_NAME(SOFT_RESET),
> +	SNOR_F_NAME(SWP_IS_VOLATILE),
> +};
> +#undef SNOR_F_NAME

Huh, clever! Took me some time to get what this is doing. But I wonder 
if there is a way to do this so we don't have to update in 2 locations 
when adding a new flag. If not, please at least add a comment on enum 
spi_nor_option_flags to remind people to also add the flag here.

But I wonder if we really need this instead of doing just:

	static const char *snor_f_names[] = {
		"HAS_SR_TB",
		"NO_OP_CHIP_ERASE",
		...
	};

Since if we have to keep this and enum spi_nor_option_flags in sync, 
might as well keep things simple.

> +
> +static const char *spi_nor_protocol_name(enum spi_nor_protocol proto)
> +{
> +	switch (proto) {
> +	case SNOR_PROTO_1_1_1:     return "1S-1S-1S";
> +	case SNOR_PROTO_1_1_2:     return "1S-1S-2S";
> +	case SNOR_PROTO_1_1_4:     return "1S-1S-4S";
> +	case SNOR_PROTO_1_1_8:     return "1S-1S-8S";
> +	case SNOR_PROTO_1_2_2:     return "1S-2S-2S";
> +	case SNOR_PROTO_1_4_4:     return "1S-4S-4S";
> +	case SNOR_PROTO_1_8_8:     return "1S-8S-8S";
> +	case SNOR_PROTO_2_2_2:     return "2S-2S-2S";
> +	case SNOR_PROTO_4_4_4:     return "4S-4S-4S";
> +	case SNOR_PROTO_8_8_8:     return "8S-8S-8S";
> +	case SNOR_PROTO_1_1_1_DTR: return "1D-1D-1D";
> +	case SNOR_PROTO_1_2_2_DTR: return "1D-2D-2D";
> +	case SNOR_PROTO_1_4_4_DTR: return "1D-4D-4D";
> +	case SNOR_PROTO_1_8_8_DTR: return "1D-8D-8D";
> +	case SNOR_PROTO_8_8_8_DTR: return "8D-8D-8D";

Okay. Though this reminds me that we have no way to specify mixed DTR 
modes like 1S-8D-8D. But that is a problem for another time.

> +	}
> +
> +	return "<unknown>";
> +}
> +
> +static void spi_nor_print_flags(struct seq_file *s, unsigned long flags,
> +				const char *const *names, int names_len)

Do you see a need to make this generic? Why not just use snor_f_names 
directly?

> +{
> +	bool sep = false;
> +	int i;
> +
> +	for (i = 0; i < sizeof(flags) * BITS_PER_BYTE; i++) {
> +		if (!(flags & BIT(i)))
> +			continue;
> +		if (sep)
> +			seq_puts(s, "|");

I have not tried running this yet, but I think putting spaces around the 
'|' would make things easier to read.

> +		sep = true;
> +		if (i < names_len && names[i])
> +			seq_puts(s, names[i]);
> +		else
> +			seq_printf(s, "1<<%d", i);

Okay, so this is in case we _don't_ keep snor_f_names in sync.

> +	}
> +}
> +
> +static int spi_nor_params_show(struct seq_file *s, void *data)
> +{
> +	struct spi_nor *nor = s->private;
> +	struct spi_nor_flash_parameter *params = nor->params;
> +	struct spi_nor_erase_map *erase_map = &params->erase_map;
> +	struct spi_nor_erase_region *region;
> +	const struct flash_info *info = nor->info;
> +	char buf[16], *str;
> +	int i;
> +
> +	seq_printf(s, "name\t\t%s\n", info->name);
> +	seq_printf(s, "id\t\t%*phN\n", info->id_len, nor->info->id);
> +	string_get_size(params->size, 1, STRING_UNITS_2, buf, sizeof(buf));
> +	seq_printf(s, "size\t\t%s\n", buf);
> +	seq_printf(s, "write size\t%u\n", params->writesize);
> +	seq_printf(s, "page size\t%u\n", params->page_size);
> +	seq_printf(s, "address width\t%u\n", nor->addr_width);
> +
> +	seq_puts(s, "flags\t\t");
> +	spi_nor_print_flags(s, nor->flags, snor_f_names, sizeof(snor_f_names));
> +	seq_puts(s, "\n");
> +
> +	seq_puts(s, "\nopcodes\n");
> +	seq_printf(s, " read\t\t%02x\n", nor->read_opcode);

Should you prefix hex numbers with "0x" to clarify their base? If 
someone sees opcode is 12 they might not be sure if this is in decimal 
or hex. I am not sure what the convention for this usually is, if there 
even is one. Same for other hex numbers printed below.

> +	seq_printf(s, "  dummy cycles\t%d\n", nor->read_dummy);
> +	seq_printf(s, " erase\t\t%02x\n", nor->erase_opcode);
> +	seq_printf(s, " program\t%02x\n", nor->program_opcode);
> +
> +	switch (nor->cmd_ext_type) {
> +	case SPI_NOR_EXT_NONE:
> +		str = "none";
> +		break;
> +	case SPI_NOR_EXT_REPEAT:
> +		str = "repeat";
> +		break;
> +	case SPI_NOR_EXT_INVERT:
> +		str = "invert";
> +		break;
> +	default:
> +		str = "<unknown>";
> +		break;
> +	}
> +	seq_printf(s, " 8D extension\t%s\n", str);

You might only want to print this if the flash is 8D capable to avoid 
confusion. But I am not sure how easy/difficult that would be, so I 
leave that up to you.

> +
> +	seq_puts(s, "\nprotocols\n");
> +	seq_printf(s, " read\t\t%s\n",
> +		   spi_nor_protocol_name(nor->read_proto));
> +	seq_printf(s, " write\t\t%s\n",
> +		   spi_nor_protocol_name(nor->write_proto));
> +	seq_printf(s, " register\t%s\n",
> +		   spi_nor_protocol_name(nor->reg_proto));
> +
> +	seq_puts(s, "\nerase commands\n");
> +	for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++) {
> +		struct spi_nor_erase_type *et = &erase_map->erase_type[i];
> +
> +		if (et->size) {
> +			string_get_size(et->size, 1, STRING_UNITS_2, buf,
> +					sizeof(buf));
> +			seq_printf(s, " %02x (%s) [%d]\n", et->opcode, buf, i);
> +		}
> +	}
> +
> +	if (!(nor->flags & SNOR_F_NO_OP_CHIP_ERASE)) {
> +		string_get_size(params->size, 1, STRING_UNITS_2, buf, sizeof(buf));
> +		seq_printf(s, " %02x (%s)\n", SPINOR_OP_CHIP_ERASE, buf);
> +	}
> +
> +	seq_puts(s, "\nsector map\n");
> +	seq_puts(s, " region              | erase mask | flags\n");
> +	seq_puts(s, " --------------------+------------+----------\n");
> +	for (region = erase_map->regions;
> +	     region;
> +	     region = spi_nor_region_next(region)) {
> +		u64 start = region->offset & ~SNOR_ERASE_FLAGS_MASK;
> +		u64 flags = region->offset & SNOR_ERASE_FLAGS_MASK;
> +		u64 end = start + region->size - 1;
> +
> +		seq_printf(s, " %08llx - %08llx |     [%c%c%c%c] | %s\n",
> +			   start, end,
> +			   flags & BIT(0) ? '0' : ' ',
> +			   flags & BIT(1) ? '1' : ' ',
> +			   flags & BIT(2) ? '2' : ' ',
> +			   flags & BIT(3) ? '3' : ' ',
> +			   flags & SNOR_OVERLAID_REGION ? "overlaid" : "");
> +	}
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(spi_nor_params);
> +
> +static void spi_nor_print_read_cmd(struct seq_file *s, u32 cap,
> +				   struct spi_nor_read_command *cmd)
> +{
> +	seq_printf(s, "%s%s\n", spi_nor_protocol_name(cmd->proto),
> +		   cap == SNOR_HWCAPS_READ_FAST ? " (fast read)" : "");
> +	seq_printf(s, "  opcode\t%02x\n", cmd->opcode);
> +	seq_printf(s, "  mode cycles\t%02x\n", cmd->num_mode_clocks);
> +	seq_printf(s, "  dummy cycles\t%02x\n", cmd->num_wait_states);
> +}
> +
> +static void spi_nor_print_pp_cmd(struct seq_file *s,
> +				 struct spi_nor_pp_command *cmd)
> +{
> +	seq_printf(s, "%s\n", spi_nor_protocol_name(cmd->proto));
> +	seq_printf(s, "  opcode\t%02x\n", cmd->opcode);
> +}
> +
> +static int spi_nor_capabilities_show(struct seq_file *s, void *data)
> +{
> +	struct spi_nor *nor = s->private;
> +	struct spi_nor_flash_parameter *params = nor->params;
> +	u32 hwcaps = params->hwcaps.mask;
> +	int i, cmd;
> +
> +	seq_puts(s, "Supported read modes by the flash\n");
> +	for (i = 0; i < sizeof(hwcaps) * BITS_PER_BYTE; i++) {
> +		if (!(hwcaps & BIT(i)))
> +			continue;
> +
> +		cmd = spi_nor_hwcaps_read2cmd(BIT(i));
> +		if (cmd < 0)
> +			continue;
> +
> +		spi_nor_print_read_cmd(s, BIT(i), &params->reads[cmd]);
> +		hwcaps &= ~BIT(i);
> +	}
> +
> +	seq_puts(s, "\nSupported page program modes by the flash\n");
> +	for (i = 0; i < sizeof(hwcaps) * BITS_PER_BYTE; i++) {
> +		if (!(hwcaps & BIT(i)))
> +			continue;
> +
> +		cmd = spi_nor_hwcaps_pp2cmd(BIT(i));
> +		if (cmd < 0)
> +			continue;
> +
> +		spi_nor_print_pp_cmd(s, &params->page_programs[cmd]);
> +		hwcaps &= ~BIT(i);
> +	}
> +
> +	if (hwcaps)
> +		seq_printf(s, "\nunknown hwcaps %x\n", hwcaps);
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(spi_nor_capabilities);
> +
> +void spi_nor_debugfs_register(struct spi_nor *nor)
> +{
> +	struct dentry *d;
> +
> +	/* Create rootdir once. Will never be deleted again. */
> +	if (!rootdir)
> +		rootdir = debugfs_create_dir("spi-nor", NULL);
> +
> +	d = debugfs_create_dir(dev_name(nor->dev), rootdir);
> +	nor->debugfs_root = d;
> +
> +	debugfs_create_file("params", 0444, d, nor, &spi_nor_params_fops);
> +	debugfs_create_file("capabilities", 0444, d, nor,
> +			    &spi_nor_capabilities_fops);
> +}
> +
> +void spi_nor_debugfs_unregister(struct spi_nor *nor)
> +{
> +	debugfs_remove(nor->debugfs_root);
> +	nor->debugfs_root = NULL;
> +}
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index 5e25a7b75ae2..7d43447768ee 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -365,6 +365,7 @@ struct spi_nor_flash_parameter;
>   * @write_proto:	the SPI protocol for write operations
>   * @reg_proto:		the SPI protocol for read_reg/write_reg/erase operations
>   * @sfdp:		the SFDP data of the flash
> + * @debugfs_root:	pointer to the debugfs directory
>   * @controller_ops:	SPI NOR controller driver specific operations.
>   * @params:		[FLASH-SPECIFIC] SPI NOR flash parameters and settings.
>   *                      The structure includes legacy flash parameters and
> @@ -394,6 +395,7 @@ struct spi_nor {
>  	u32			flags;
>  	enum spi_nor_cmd_ext	cmd_ext_type;
>  	struct sfdp		*sfdp;
> +	struct dentry		*debugfs_root;
>  
>  	const struct spi_nor_controller_ops *controller_ops;
>  
> -- 
> 2.30.2
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH RFC v1 2/2] mtd: spi-nor: expose internal parameters via debugfs
@ 2022-04-20  5:23     ` Pratyush Yadav
  0 siblings, 0 replies; 10+ messages in thread
From: Pratyush Yadav @ 2022-04-20  5:23 UTC (permalink / raw)
  To: Michael Walle
  Cc: Tudor Ambarus, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, linux-mtd, linux-kernel

Hi Michael,

On 18/04/22 02:10PM, Michael Walle wrote:
> There is no way to gather all information to verify support for a new
> flash chip. Also if you want to convert an existing flash chip to the
> new SFDP parsing, there is not enough information to determine if the
> flash will work like before. To ease this development, expose internal
> parameters via the debugfs.

A big +1 for this patch from me. I have too often added prints in the 
driver to find out all this information. This won't be very useful in 
case the probe fails, but I don't suppose we can do much about that.

> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/Makefile  |   1 +
>  drivers/mtd/spi-nor/core.c    |   4 +
>  drivers/mtd/spi-nor/core.h    |   8 ++
>  drivers/mtd/spi-nor/debugfs.c | 241 ++++++++++++++++++++++++++++++++++
>  include/linux/mtd/spi-nor.h   |   2 +
>  5 files changed, 256 insertions(+)
>  create mode 100644 drivers/mtd/spi-nor/debugfs.c
> 
> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
> index 6b904e439372..e347b435a038 100644
> --- a/drivers/mtd/spi-nor/Makefile
> +++ b/drivers/mtd/spi-nor/Makefile
> @@ -17,6 +17,7 @@ spi-nor-objs			+= sst.o
>  spi-nor-objs			+= winbond.o
>  spi-nor-objs			+= xilinx.o
>  spi-nor-objs			+= xmc.o
> +spi-nor-$(CONFIG_DEBUG_FS)	+= debugfs.o
>  obj-$(CONFIG_MTD_SPI_NOR)	+= spi-nor.o
>  
>  obj-$(CONFIG_MTD_SPI_NOR)	+= controllers/
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 80d65cfcb88d..302331695d96 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3117,6 +3117,8 @@ static int spi_nor_probe(struct spi_mem *spimem)
>  	if (ret)
>  		return ret;
>  
> +	spi_nor_debugfs_register(nor);

I think you should register this after mtd_device_register() to be sure 
the probe would not fail with these debugfs entries still in place. Or 
clean up before returning errors in the code below. Otherwise access to 
the debugfs entires would cause you to access a nor struct that is no 
longer there.

> +
>  	/*
>  	 * None of the existing parts have > 512B pages, but let's play safe
>  	 * and add this logic so that if anyone ever adds support for such
> @@ -3148,6 +3150,8 @@ static int spi_nor_remove(struct spi_mem *spimem)
>  {
>  	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
>  
> +	spi_nor_debugfs_unregister(nor);
> +
>  	spi_nor_restore(nor);
>  
>  	/* Clean up MTD stuff. */
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 81c4bb7d3193..d042d745a1f6 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -589,4 +589,12 @@ static inline struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)
>  	return container_of(mtd, struct spi_nor, mtd);
>  }
>  
> +#ifdef CONFIG_DEBUG_FS
> +void spi_nor_debugfs_register(struct spi_nor *nor);
> +void spi_nor_debugfs_unregister(struct spi_nor *nor);
> +#else
> +static inline void spi_nor_debugfs_register(struct spi_nor *nor) {}
> +static inline void spi_nor_debugfs_unregister(struct spi_nor *nor) {}
> +#endif
> +
>  #endif /* __LINUX_MTD_SPI_NOR_INTERNAL_H */
> diff --git a/drivers/mtd/spi-nor/debugfs.c b/drivers/mtd/spi-nor/debugfs.c
> new file mode 100644
> index 000000000000..61d6d90eda13
> --- /dev/null
> +++ b/drivers/mtd/spi-nor/debugfs.c
> @@ -0,0 +1,241 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/mtd/spi-nor.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/spi-mem.h>
> +#include <linux/debugfs.h>
> +
> +#include "core.h"
> +
> +static struct dentry *rootdir;
> +
> +#define SNOR_F_NAME(name) [ilog2(SNOR_F_##name)] = #name
> +static const char *const snor_f_names[] = {
> +	SNOR_F_NAME(HAS_SR_TB),
> +	SNOR_F_NAME(NO_OP_CHIP_ERASE),
> +	SNOR_F_NAME(BROKEN_RESET),
> +	SNOR_F_NAME(4B_OPCODES),
> +	SNOR_F_NAME(HAS_4BAIT),
> +	SNOR_F_NAME(HAS_LOCK),
> +	SNOR_F_NAME(HAS_16BIT_SR),
> +	SNOR_F_NAME(NO_READ_CR),
> +	SNOR_F_NAME(HAS_SR_TB_BIT6),
> +	SNOR_F_NAME(HAS_4BIT_BP),
> +	SNOR_F_NAME(HAS_SR_BP3_BIT6),
> +	SNOR_F_NAME(IO_MODE_EN_VOLATILE),
> +	SNOR_F_NAME(SOFT_RESET),
> +	SNOR_F_NAME(SWP_IS_VOLATILE),
> +};
> +#undef SNOR_F_NAME

Huh, clever! Took me some time to get what this is doing. But I wonder 
if there is a way to do this so we don't have to update in 2 locations 
when adding a new flag. If not, please at least add a comment on enum 
spi_nor_option_flags to remind people to also add the flag here.

But I wonder if we really need this instead of doing just:

	static const char *snor_f_names[] = {
		"HAS_SR_TB",
		"NO_OP_CHIP_ERASE",
		...
	};

Since if we have to keep this and enum spi_nor_option_flags in sync, 
might as well keep things simple.

> +
> +static const char *spi_nor_protocol_name(enum spi_nor_protocol proto)
> +{
> +	switch (proto) {
> +	case SNOR_PROTO_1_1_1:     return "1S-1S-1S";
> +	case SNOR_PROTO_1_1_2:     return "1S-1S-2S";
> +	case SNOR_PROTO_1_1_4:     return "1S-1S-4S";
> +	case SNOR_PROTO_1_1_8:     return "1S-1S-8S";
> +	case SNOR_PROTO_1_2_2:     return "1S-2S-2S";
> +	case SNOR_PROTO_1_4_4:     return "1S-4S-4S";
> +	case SNOR_PROTO_1_8_8:     return "1S-8S-8S";
> +	case SNOR_PROTO_2_2_2:     return "2S-2S-2S";
> +	case SNOR_PROTO_4_4_4:     return "4S-4S-4S";
> +	case SNOR_PROTO_8_8_8:     return "8S-8S-8S";
> +	case SNOR_PROTO_1_1_1_DTR: return "1D-1D-1D";
> +	case SNOR_PROTO_1_2_2_DTR: return "1D-2D-2D";
> +	case SNOR_PROTO_1_4_4_DTR: return "1D-4D-4D";
> +	case SNOR_PROTO_1_8_8_DTR: return "1D-8D-8D";
> +	case SNOR_PROTO_8_8_8_DTR: return "8D-8D-8D";

Okay. Though this reminds me that we have no way to specify mixed DTR 
modes like 1S-8D-8D. But that is a problem for another time.

> +	}
> +
> +	return "<unknown>";
> +}
> +
> +static void spi_nor_print_flags(struct seq_file *s, unsigned long flags,
> +				const char *const *names, int names_len)

Do you see a need to make this generic? Why not just use snor_f_names 
directly?

> +{
> +	bool sep = false;
> +	int i;
> +
> +	for (i = 0; i < sizeof(flags) * BITS_PER_BYTE; i++) {
> +		if (!(flags & BIT(i)))
> +			continue;
> +		if (sep)
> +			seq_puts(s, "|");

I have not tried running this yet, but I think putting spaces around the 
'|' would make things easier to read.

> +		sep = true;
> +		if (i < names_len && names[i])
> +			seq_puts(s, names[i]);
> +		else
> +			seq_printf(s, "1<<%d", i);

Okay, so this is in case we _don't_ keep snor_f_names in sync.

> +	}
> +}
> +
> +static int spi_nor_params_show(struct seq_file *s, void *data)
> +{
> +	struct spi_nor *nor = s->private;
> +	struct spi_nor_flash_parameter *params = nor->params;
> +	struct spi_nor_erase_map *erase_map = &params->erase_map;
> +	struct spi_nor_erase_region *region;
> +	const struct flash_info *info = nor->info;
> +	char buf[16], *str;
> +	int i;
> +
> +	seq_printf(s, "name\t\t%s\n", info->name);
> +	seq_printf(s, "id\t\t%*phN\n", info->id_len, nor->info->id);
> +	string_get_size(params->size, 1, STRING_UNITS_2, buf, sizeof(buf));
> +	seq_printf(s, "size\t\t%s\n", buf);
> +	seq_printf(s, "write size\t%u\n", params->writesize);
> +	seq_printf(s, "page size\t%u\n", params->page_size);
> +	seq_printf(s, "address width\t%u\n", nor->addr_width);
> +
> +	seq_puts(s, "flags\t\t");
> +	spi_nor_print_flags(s, nor->flags, snor_f_names, sizeof(snor_f_names));
> +	seq_puts(s, "\n");
> +
> +	seq_puts(s, "\nopcodes\n");
> +	seq_printf(s, " read\t\t%02x\n", nor->read_opcode);

Should you prefix hex numbers with "0x" to clarify their base? If 
someone sees opcode is 12 they might not be sure if this is in decimal 
or hex. I am not sure what the convention for this usually is, if there 
even is one. Same for other hex numbers printed below.

> +	seq_printf(s, "  dummy cycles\t%d\n", nor->read_dummy);
> +	seq_printf(s, " erase\t\t%02x\n", nor->erase_opcode);
> +	seq_printf(s, " program\t%02x\n", nor->program_opcode);
> +
> +	switch (nor->cmd_ext_type) {
> +	case SPI_NOR_EXT_NONE:
> +		str = "none";
> +		break;
> +	case SPI_NOR_EXT_REPEAT:
> +		str = "repeat";
> +		break;
> +	case SPI_NOR_EXT_INVERT:
> +		str = "invert";
> +		break;
> +	default:
> +		str = "<unknown>";
> +		break;
> +	}
> +	seq_printf(s, " 8D extension\t%s\n", str);

You might only want to print this if the flash is 8D capable to avoid 
confusion. But I am not sure how easy/difficult that would be, so I 
leave that up to you.

> +
> +	seq_puts(s, "\nprotocols\n");
> +	seq_printf(s, " read\t\t%s\n",
> +		   spi_nor_protocol_name(nor->read_proto));
> +	seq_printf(s, " write\t\t%s\n",
> +		   spi_nor_protocol_name(nor->write_proto));
> +	seq_printf(s, " register\t%s\n",
> +		   spi_nor_protocol_name(nor->reg_proto));
> +
> +	seq_puts(s, "\nerase commands\n");
> +	for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++) {
> +		struct spi_nor_erase_type *et = &erase_map->erase_type[i];
> +
> +		if (et->size) {
> +			string_get_size(et->size, 1, STRING_UNITS_2, buf,
> +					sizeof(buf));
> +			seq_printf(s, " %02x (%s) [%d]\n", et->opcode, buf, i);
> +		}
> +	}
> +
> +	if (!(nor->flags & SNOR_F_NO_OP_CHIP_ERASE)) {
> +		string_get_size(params->size, 1, STRING_UNITS_2, buf, sizeof(buf));
> +		seq_printf(s, " %02x (%s)\n", SPINOR_OP_CHIP_ERASE, buf);
> +	}
> +
> +	seq_puts(s, "\nsector map\n");
> +	seq_puts(s, " region              | erase mask | flags\n");
> +	seq_puts(s, " --------------------+------------+----------\n");
> +	for (region = erase_map->regions;
> +	     region;
> +	     region = spi_nor_region_next(region)) {
> +		u64 start = region->offset & ~SNOR_ERASE_FLAGS_MASK;
> +		u64 flags = region->offset & SNOR_ERASE_FLAGS_MASK;
> +		u64 end = start + region->size - 1;
> +
> +		seq_printf(s, " %08llx - %08llx |     [%c%c%c%c] | %s\n",
> +			   start, end,
> +			   flags & BIT(0) ? '0' : ' ',
> +			   flags & BIT(1) ? '1' : ' ',
> +			   flags & BIT(2) ? '2' : ' ',
> +			   flags & BIT(3) ? '3' : ' ',
> +			   flags & SNOR_OVERLAID_REGION ? "overlaid" : "");
> +	}
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(spi_nor_params);
> +
> +static void spi_nor_print_read_cmd(struct seq_file *s, u32 cap,
> +				   struct spi_nor_read_command *cmd)
> +{
> +	seq_printf(s, "%s%s\n", spi_nor_protocol_name(cmd->proto),
> +		   cap == SNOR_HWCAPS_READ_FAST ? " (fast read)" : "");
> +	seq_printf(s, "  opcode\t%02x\n", cmd->opcode);
> +	seq_printf(s, "  mode cycles\t%02x\n", cmd->num_mode_clocks);
> +	seq_printf(s, "  dummy cycles\t%02x\n", cmd->num_wait_states);
> +}
> +
> +static void spi_nor_print_pp_cmd(struct seq_file *s,
> +				 struct spi_nor_pp_command *cmd)
> +{
> +	seq_printf(s, "%s\n", spi_nor_protocol_name(cmd->proto));
> +	seq_printf(s, "  opcode\t%02x\n", cmd->opcode);
> +}
> +
> +static int spi_nor_capabilities_show(struct seq_file *s, void *data)
> +{
> +	struct spi_nor *nor = s->private;
> +	struct spi_nor_flash_parameter *params = nor->params;
> +	u32 hwcaps = params->hwcaps.mask;
> +	int i, cmd;
> +
> +	seq_puts(s, "Supported read modes by the flash\n");
> +	for (i = 0; i < sizeof(hwcaps) * BITS_PER_BYTE; i++) {
> +		if (!(hwcaps & BIT(i)))
> +			continue;
> +
> +		cmd = spi_nor_hwcaps_read2cmd(BIT(i));
> +		if (cmd < 0)
> +			continue;
> +
> +		spi_nor_print_read_cmd(s, BIT(i), &params->reads[cmd]);
> +		hwcaps &= ~BIT(i);
> +	}
> +
> +	seq_puts(s, "\nSupported page program modes by the flash\n");
> +	for (i = 0; i < sizeof(hwcaps) * BITS_PER_BYTE; i++) {
> +		if (!(hwcaps & BIT(i)))
> +			continue;
> +
> +		cmd = spi_nor_hwcaps_pp2cmd(BIT(i));
> +		if (cmd < 0)
> +			continue;
> +
> +		spi_nor_print_pp_cmd(s, &params->page_programs[cmd]);
> +		hwcaps &= ~BIT(i);
> +	}
> +
> +	if (hwcaps)
> +		seq_printf(s, "\nunknown hwcaps %x\n", hwcaps);
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(spi_nor_capabilities);
> +
> +void spi_nor_debugfs_register(struct spi_nor *nor)
> +{
> +	struct dentry *d;
> +
> +	/* Create rootdir once. Will never be deleted again. */
> +	if (!rootdir)
> +		rootdir = debugfs_create_dir("spi-nor", NULL);
> +
> +	d = debugfs_create_dir(dev_name(nor->dev), rootdir);
> +	nor->debugfs_root = d;
> +
> +	debugfs_create_file("params", 0444, d, nor, &spi_nor_params_fops);
> +	debugfs_create_file("capabilities", 0444, d, nor,
> +			    &spi_nor_capabilities_fops);
> +}
> +
> +void spi_nor_debugfs_unregister(struct spi_nor *nor)
> +{
> +	debugfs_remove(nor->debugfs_root);
> +	nor->debugfs_root = NULL;
> +}
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index 5e25a7b75ae2..7d43447768ee 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -365,6 +365,7 @@ struct spi_nor_flash_parameter;
>   * @write_proto:	the SPI protocol for write operations
>   * @reg_proto:		the SPI protocol for read_reg/write_reg/erase operations
>   * @sfdp:		the SFDP data of the flash
> + * @debugfs_root:	pointer to the debugfs directory
>   * @controller_ops:	SPI NOR controller driver specific operations.
>   * @params:		[FLASH-SPECIFIC] SPI NOR flash parameters and settings.
>   *                      The structure includes legacy flash parameters and
> @@ -394,6 +395,7 @@ struct spi_nor {
>  	u32			flags;
>  	enum spi_nor_cmd_ext	cmd_ext_type;
>  	struct sfdp		*sfdp;
> +	struct dentry		*debugfs_root;
>  
>  	const struct spi_nor_controller_ops *controller_ops;
>  
> -- 
> 2.30.2
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH RFC v1 2/2] mtd: spi-nor: expose internal parameters via debugfs
  2022-04-20  5:23     ` Pratyush Yadav
@ 2022-04-20  9:18       ` Michael Walle
  -1 siblings, 0 replies; 10+ messages in thread
From: Michael Walle @ 2022-04-20  9:18 UTC (permalink / raw)
  To: Pratyush Yadav
  Cc: Tudor Ambarus, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, linux-mtd, linux-kernel

Am 2022-04-20 07:23, schrieb Pratyush Yadav:
> On 18/04/22 02:10PM, Michael Walle wrote:
>> There is no way to gather all information to verify support for a new
>> flash chip. Also if you want to convert an existing flash chip to the
>> new SFDP parsing, there is not enough information to determine if the
>> flash will work like before. To ease this development, expose internal
>> parameters via the debugfs.
> 
> A big +1 for this patch from me. I have too often added prints in the
> driver to find out all this information. This won't be very useful in
> case the probe fails, but I don't suppose we can do much about that.

That might get better with the "generic sfdp driver" fallback though.

>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>  drivers/mtd/spi-nor/Makefile  |   1 +
>>  drivers/mtd/spi-nor/core.c    |   4 +
>>  drivers/mtd/spi-nor/core.h    |   8 ++
>>  drivers/mtd/spi-nor/debugfs.c | 241 
>> ++++++++++++++++++++++++++++++++++
>>  include/linux/mtd/spi-nor.h   |   2 +
>>  5 files changed, 256 insertions(+)
>>  create mode 100644 drivers/mtd/spi-nor/debugfs.c
>> 
>> diff --git a/drivers/mtd/spi-nor/Makefile 
>> b/drivers/mtd/spi-nor/Makefile
>> index 6b904e439372..e347b435a038 100644
>> --- a/drivers/mtd/spi-nor/Makefile
>> +++ b/drivers/mtd/spi-nor/Makefile
>> @@ -17,6 +17,7 @@ spi-nor-objs			+= sst.o
>>  spi-nor-objs			+= winbond.o
>>  spi-nor-objs			+= xilinx.o
>>  spi-nor-objs			+= xmc.o
>> +spi-nor-$(CONFIG_DEBUG_FS)	+= debugfs.o
>>  obj-$(CONFIG_MTD_SPI_NOR)	+= spi-nor.o
>> 
>>  obj-$(CONFIG_MTD_SPI_NOR)	+= controllers/
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 80d65cfcb88d..302331695d96 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -3117,6 +3117,8 @@ static int spi_nor_probe(struct spi_mem *spimem)
>>  	if (ret)
>>  		return ret;
>> 
>> +	spi_nor_debugfs_register(nor);
> 
> I think you should register this after mtd_device_register() to be sure
> the probe would not fail with these debugfs entries still in place. Or
> clean up before returning errors in the code below. Otherwise access to
> the debugfs entires would cause you to access a nor struct that is no
> longer there.

Ohh, you are right. I'll probaly drop the unregister altogether and use
devm_add_action().

>> +
>>  	/*
>>  	 * None of the existing parts have > 512B pages, but let's play safe
>>  	 * and add this logic so that if anyone ever adds support for such
>> @@ -3148,6 +3150,8 @@ static int spi_nor_remove(struct spi_mem 
>> *spimem)
>>  {
>>  	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
>> 
>> +	spi_nor_debugfs_unregister(nor);
>> +
>>  	spi_nor_restore(nor);
>> 
>>  	/* Clean up MTD stuff. */
>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>> index 81c4bb7d3193..d042d745a1f6 100644
>> --- a/drivers/mtd/spi-nor/core.h
>> +++ b/drivers/mtd/spi-nor/core.h
>> @@ -589,4 +589,12 @@ static inline struct spi_nor 
>> *mtd_to_spi_nor(struct mtd_info *mtd)
>>  	return container_of(mtd, struct spi_nor, mtd);
>>  }
>> 
>> +#ifdef CONFIG_DEBUG_FS
>> +void spi_nor_debugfs_register(struct spi_nor *nor);
>> +void spi_nor_debugfs_unregister(struct spi_nor *nor);
>> +#else
>> +static inline void spi_nor_debugfs_register(struct spi_nor *nor) {}
>> +static inline void spi_nor_debugfs_unregister(struct spi_nor *nor) {}
>> +#endif
>> +
>>  #endif /* __LINUX_MTD_SPI_NOR_INTERNAL_H */
>> diff --git a/drivers/mtd/spi-nor/debugfs.c 
>> b/drivers/mtd/spi-nor/debugfs.c
>> new file mode 100644
>> index 000000000000..61d6d90eda13
>> --- /dev/null
>> +++ b/drivers/mtd/spi-nor/debugfs.c
>> @@ -0,0 +1,241 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include <linux/mtd/spi-nor.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/spi/spi-mem.h>
>> +#include <linux/debugfs.h>
>> +
>> +#include "core.h"
>> +
>> +static struct dentry *rootdir;
>> +
>> +#define SNOR_F_NAME(name) [ilog2(SNOR_F_##name)] = #name
>> +static const char *const snor_f_names[] = {
>> +	SNOR_F_NAME(HAS_SR_TB),
>> +	SNOR_F_NAME(NO_OP_CHIP_ERASE),
>> +	SNOR_F_NAME(BROKEN_RESET),
>> +	SNOR_F_NAME(4B_OPCODES),
>> +	SNOR_F_NAME(HAS_4BAIT),
>> +	SNOR_F_NAME(HAS_LOCK),
>> +	SNOR_F_NAME(HAS_16BIT_SR),
>> +	SNOR_F_NAME(NO_READ_CR),
>> +	SNOR_F_NAME(HAS_SR_TB_BIT6),
>> +	SNOR_F_NAME(HAS_4BIT_BP),
>> +	SNOR_F_NAME(HAS_SR_BP3_BIT6),
>> +	SNOR_F_NAME(IO_MODE_EN_VOLATILE),
>> +	SNOR_F_NAME(SOFT_RESET),
>> +	SNOR_F_NAME(SWP_IS_VOLATILE),
>> +};
>> +#undef SNOR_F_NAME
> 
> Huh, clever! Took me some time to get what this is doing. But I wonder
> if there is a way to do this so we don't have to update in 2 locations
> when adding a new flag. If not, please at least add a comment on enum
> spi_nor_option_flags to remind people to also add the flag here.

Sure can add a comment.

> 
> But I wonder if we really need this instead of doing just:
> 
> 	static const char *snor_f_names[] = {
> 		"HAS_SR_TB",
> 		"NO_OP_CHIP_ERASE",
> 		...
> 	};
> 
> Since if we have to keep this and enum spi_nor_option_flags in sync,
> might as well keep things simple.

But then the compiler wont help us catching renames and reorders.

>> +
>> +static const char *spi_nor_protocol_name(enum spi_nor_protocol proto)
>> +{
>> +	switch (proto) {
>> +	case SNOR_PROTO_1_1_1:     return "1S-1S-1S";
>> +	case SNOR_PROTO_1_1_2:     return "1S-1S-2S";
>> +	case SNOR_PROTO_1_1_4:     return "1S-1S-4S";
>> +	case SNOR_PROTO_1_1_8:     return "1S-1S-8S";
>> +	case SNOR_PROTO_1_2_2:     return "1S-2S-2S";
>> +	case SNOR_PROTO_1_4_4:     return "1S-4S-4S";
>> +	case SNOR_PROTO_1_8_8:     return "1S-8S-8S";
>> +	case SNOR_PROTO_2_2_2:     return "2S-2S-2S";
>> +	case SNOR_PROTO_4_4_4:     return "4S-4S-4S";
>> +	case SNOR_PROTO_8_8_8:     return "8S-8S-8S";
>> +	case SNOR_PROTO_1_1_1_DTR: return "1D-1D-1D";
>> +	case SNOR_PROTO_1_2_2_DTR: return "1D-2D-2D";
>> +	case SNOR_PROTO_1_4_4_DTR: return "1D-4D-4D";
>> +	case SNOR_PROTO_1_8_8_DTR: return "1D-8D-8D";
>> +	case SNOR_PROTO_8_8_8_DTR: return "8D-8D-8D";
> 
> Okay. Though this reminds me that we have no way to specify mixed DTR
> modes like 1S-8D-8D. But that is a problem for another time.
> 
>> +	}
>> +
>> +	return "<unknown>";
>> +}
>> +
>> +static void spi_nor_print_flags(struct seq_file *s, unsigned long 
>> flags,
>> +				const char *const *names, int names_len)
> 
> Do you see a need to make this generic? Why not just use snor_f_names
> directly?

There might be more flags in the future, so why should I make
this unnecessarily specific? ;)

>> +{
>> +	bool sep = false;
>> +	int i;
>> +
>> +	for (i = 0; i < sizeof(flags) * BITS_PER_BYTE; i++) {
>> +		if (!(flags & BIT(i)))
>> +			continue;
>> +		if (sep)
>> +			seq_puts(s, "|");
> 
> I have not tried running this yet, but I think putting spaces around 
> the
> '|' would make things easier to read.

Yeah and there are also no linewraps, it's just for debugging.
No need to win a beauty contest ;)

flags		HAS_16BIT_SR|NO_READ_CR|SOFT_RESET
vs
flags		HAS_16BIT_SR | NO_READ_CR | SOFT_RESET

I'm fine with either. Can add the spaces.

>> +		sep = true;
>> +		if (i < names_len && names[i])
>> +			seq_puts(s, names[i]);
>> +		else
>> +			seq_printf(s, "1<<%d", i);
> 
> Okay, so this is in case we _don't_ keep snor_f_names in sync.
> 
>> +	}
>> +}
>> +
>> +static int spi_nor_params_show(struct seq_file *s, void *data)
>> +{
>> +	struct spi_nor *nor = s->private;
>> +	struct spi_nor_flash_parameter *params = nor->params;
>> +	struct spi_nor_erase_map *erase_map = &params->erase_map;
>> +	struct spi_nor_erase_region *region;
>> +	const struct flash_info *info = nor->info;
>> +	char buf[16], *str;
>> +	int i;
>> +
>> +	seq_printf(s, "name\t\t%s\n", info->name);
>> +	seq_printf(s, "id\t\t%*phN\n", info->id_len, nor->info->id);
>> +	string_get_size(params->size, 1, STRING_UNITS_2, buf, sizeof(buf));
>> +	seq_printf(s, "size\t\t%s\n", buf);
>> +	seq_printf(s, "write size\t%u\n", params->writesize);
>> +	seq_printf(s, "page size\t%u\n", params->page_size);
>> +	seq_printf(s, "address width\t%u\n", nor->addr_width);
>> +
>> +	seq_puts(s, "flags\t\t");
>> +	spi_nor_print_flags(s, nor->flags, snor_f_names, 
>> sizeof(snor_f_names));
>> +	seq_puts(s, "\n");
>> +
>> +	seq_puts(s, "\nopcodes\n");
>> +	seq_printf(s, " read\t\t%02x\n", nor->read_opcode);
> 
> Should you prefix hex numbers with "0x" to clarify their base? If
> someone sees opcode is 12 they might not be sure if this is in decimal
> or hex. I am not sure what the convention for this usually is, if there
> even is one. Same for other hex numbers printed below.

Yeah, I wasn't sure about that either. The target audience are
developers so I just dropped the prefix (or suffix 'h'). I'll
re-add the prefix, though.

>> +	seq_printf(s, "  dummy cycles\t%d\n", nor->read_dummy);
>> +	seq_printf(s, " erase\t\t%02x\n", nor->erase_opcode);
>> +	seq_printf(s, " program\t%02x\n", nor->program_opcode);

.. esp. because the bases are mixed here.

>> +
>> +	switch (nor->cmd_ext_type) {
>> +	case SPI_NOR_EXT_NONE:
>> +		str = "none";
>> +		break;
>> +	case SPI_NOR_EXT_REPEAT:
>> +		str = "repeat";
>> +		break;
>> +	case SPI_NOR_EXT_INVERT:
>> +		str = "invert";
>> +		break;
>> +	default:
>> +		str = "<unknown>";
>> +		break;
>> +	}
>> +	seq_printf(s, " 8D extension\t%s\n", str);
> 
> You might only want to print this if the flash is 8D capable to avoid
> confusion. But I am not sure how easy/difficult that would be, so I
> leave that up to you.

This is parsed from the SFDP regardless of the mode, so it might be
helpful even if it's unused. I reckon upon the knowledge of the
developer :)

>> +
>> +	seq_puts(s, "\nprotocols\n");
>> +	seq_printf(s, " read\t\t%s\n",
>> +		   spi_nor_protocol_name(nor->read_proto));
>> +	seq_printf(s, " write\t\t%s\n",
>> +		   spi_nor_protocol_name(nor->write_proto));
>> +	seq_printf(s, " register\t%s\n",
>> +		   spi_nor_protocol_name(nor->reg_proto));
>> +
>> +	seq_puts(s, "\nerase commands\n");
>> +	for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++) {
>> +		struct spi_nor_erase_type *et = &erase_map->erase_type[i];
>> +
>> +		if (et->size) {
>> +			string_get_size(et->size, 1, STRING_UNITS_2, buf,
>> +					sizeof(buf));
>> +			seq_printf(s, " %02x (%s) [%d]\n", et->opcode, buf, i);
>> +		}
>> +	}
>> +
>> +	if (!(nor->flags & SNOR_F_NO_OP_CHIP_ERASE)) {
>> +		string_get_size(params->size, 1, STRING_UNITS_2, buf, sizeof(buf));
>> +		seq_printf(s, " %02x (%s)\n", SPINOR_OP_CHIP_ERASE, buf);
>> +	}
>> +
>> +	seq_puts(s, "\nsector map\n");
>> +	seq_puts(s, " region              | erase mask | flags\n");
>> +	seq_puts(s, " --------------------+------------+----------\n");
>> +	for (region = erase_map->regions;
>> +	     region;
>> +	     region = spi_nor_region_next(region)) {
>> +		u64 start = region->offset & ~SNOR_ERASE_FLAGS_MASK;
>> +		u64 flags = region->offset & SNOR_ERASE_FLAGS_MASK;
>> +		u64 end = start + region->size - 1;
>> +
>> +		seq_printf(s, " %08llx - %08llx |     [%c%c%c%c] | %s\n",
>> +			   start, end,
>> +			   flags & BIT(0) ? '0' : ' ',
>> +			   flags & BIT(1) ? '1' : ' ',
>> +			   flags & BIT(2) ? '2' : ' ',
>> +			   flags & BIT(3) ? '3' : ' ',
>> +			   flags & SNOR_OVERLAID_REGION ? "overlaid" : "");
>> +	}
>> +
>> +	return 0;
>> +}
>> +DEFINE_SHOW_ATTRIBUTE(spi_nor_params);
>> +
>> +static void spi_nor_print_read_cmd(struct seq_file *s, u32 cap,
>> +				   struct spi_nor_read_command *cmd)
>> +{
>> +	seq_printf(s, "%s%s\n", spi_nor_protocol_name(cmd->proto),
>> +		   cap == SNOR_HWCAPS_READ_FAST ? " (fast read)" : "");
>> +	seq_printf(s, "  opcode\t%02x\n", cmd->opcode);
>> +	seq_printf(s, "  mode cycles\t%02x\n", cmd->num_mode_clocks);
>> +	seq_printf(s, "  dummy cycles\t%02x\n", cmd->num_wait_states);
>> +}
>> +
>> +static void spi_nor_print_pp_cmd(struct seq_file *s,
>> +				 struct spi_nor_pp_command *cmd)
>> +{
>> +	seq_printf(s, "%s\n", spi_nor_protocol_name(cmd->proto));
>> +	seq_printf(s, "  opcode\t%02x\n", cmd->opcode);
>> +}
>> +
>> +static int spi_nor_capabilities_show(struct seq_file *s, void *data)
>> +{
>> +	struct spi_nor *nor = s->private;
>> +	struct spi_nor_flash_parameter *params = nor->params;
>> +	u32 hwcaps = params->hwcaps.mask;
>> +	int i, cmd;
>> +
>> +	seq_puts(s, "Supported read modes by the flash\n");
>> +	for (i = 0; i < sizeof(hwcaps) * BITS_PER_BYTE; i++) {
>> +		if (!(hwcaps & BIT(i)))
>> +			continue;
>> +
>> +		cmd = spi_nor_hwcaps_read2cmd(BIT(i));
>> +		if (cmd < 0)
>> +			continue;
>> +
>> +		spi_nor_print_read_cmd(s, BIT(i), &params->reads[cmd]);
>> +		hwcaps &= ~BIT(i);
>> +	}
>> +
>> +	seq_puts(s, "\nSupported page program modes by the flash\n");
>> +	for (i = 0; i < sizeof(hwcaps) * BITS_PER_BYTE; i++) {
>> +		if (!(hwcaps & BIT(i)))
>> +			continue;
>> +
>> +		cmd = spi_nor_hwcaps_pp2cmd(BIT(i));
>> +		if (cmd < 0)
>> +			continue;
>> +
>> +		spi_nor_print_pp_cmd(s, &params->page_programs[cmd]);
>> +		hwcaps &= ~BIT(i);
>> +	}
>> +
>> +	if (hwcaps)
>> +		seq_printf(s, "\nunknown hwcaps %x\n", hwcaps);
>> +
>> +	return 0;
>> +}
>> +DEFINE_SHOW_ATTRIBUTE(spi_nor_capabilities);
>> +
>> +void spi_nor_debugfs_register(struct spi_nor *nor)
>> +{
>> +	struct dentry *d;
>> +
>> +	/* Create rootdir once. Will never be deleted again. */
>> +	if (!rootdir)
>> +		rootdir = debugfs_create_dir("spi-nor", NULL);
>> +
>> +	d = debugfs_create_dir(dev_name(nor->dev), rootdir);
>> +	nor->debugfs_root = d;
>> +
>> +	debugfs_create_file("params", 0444, d, nor, &spi_nor_params_fops);
>> +	debugfs_create_file("capabilities", 0444, d, nor,
>> +			    &spi_nor_capabilities_fops);
>> +}
>> +
>> +void spi_nor_debugfs_unregister(struct spi_nor *nor)
>> +{
>> +	debugfs_remove(nor->debugfs_root);
>> +	nor->debugfs_root = NULL;
>> +}
>> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
>> index 5e25a7b75ae2..7d43447768ee 100644
>> --- a/include/linux/mtd/spi-nor.h
>> +++ b/include/linux/mtd/spi-nor.h
>> @@ -365,6 +365,7 @@ struct spi_nor_flash_parameter;
>>   * @write_proto:	the SPI protocol for write operations
>>   * @reg_proto:		the SPI protocol for read_reg/write_reg/erase 
>> operations
>>   * @sfdp:		the SFDP data of the flash
>> + * @debugfs_root:	pointer to the debugfs directory
>>   * @controller_ops:	SPI NOR controller driver specific operations.
>>   * @params:		[FLASH-SPECIFIC] SPI NOR flash parameters and settings.
>>   *                      The structure includes legacy flash 
>> parameters and
>> @@ -394,6 +395,7 @@ struct spi_nor {
>>  	u32			flags;
>>  	enum spi_nor_cmd_ext	cmd_ext_type;
>>  	struct sfdp		*sfdp;
>> +	struct dentry		*debugfs_root;
>> 
>>  	const struct spi_nor_controller_ops *controller_ops;
>> 
>> --
>> 2.30.2
>> 

-- 
-michael

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH RFC v1 2/2] mtd: spi-nor: expose internal parameters via debugfs
@ 2022-04-20  9:18       ` Michael Walle
  0 siblings, 0 replies; 10+ messages in thread
From: Michael Walle @ 2022-04-20  9:18 UTC (permalink / raw)
  To: Pratyush Yadav
  Cc: Tudor Ambarus, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, linux-mtd, linux-kernel

Am 2022-04-20 07:23, schrieb Pratyush Yadav:
> On 18/04/22 02:10PM, Michael Walle wrote:
>> There is no way to gather all information to verify support for a new
>> flash chip. Also if you want to convert an existing flash chip to the
>> new SFDP parsing, there is not enough information to determine if the
>> flash will work like before. To ease this development, expose internal
>> parameters via the debugfs.
> 
> A big +1 for this patch from me. I have too often added prints in the
> driver to find out all this information. This won't be very useful in
> case the probe fails, but I don't suppose we can do much about that.

That might get better with the "generic sfdp driver" fallback though.

>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>  drivers/mtd/spi-nor/Makefile  |   1 +
>>  drivers/mtd/spi-nor/core.c    |   4 +
>>  drivers/mtd/spi-nor/core.h    |   8 ++
>>  drivers/mtd/spi-nor/debugfs.c | 241 
>> ++++++++++++++++++++++++++++++++++
>>  include/linux/mtd/spi-nor.h   |   2 +
>>  5 files changed, 256 insertions(+)
>>  create mode 100644 drivers/mtd/spi-nor/debugfs.c
>> 
>> diff --git a/drivers/mtd/spi-nor/Makefile 
>> b/drivers/mtd/spi-nor/Makefile
>> index 6b904e439372..e347b435a038 100644
>> --- a/drivers/mtd/spi-nor/Makefile
>> +++ b/drivers/mtd/spi-nor/Makefile
>> @@ -17,6 +17,7 @@ spi-nor-objs			+= sst.o
>>  spi-nor-objs			+= winbond.o
>>  spi-nor-objs			+= xilinx.o
>>  spi-nor-objs			+= xmc.o
>> +spi-nor-$(CONFIG_DEBUG_FS)	+= debugfs.o
>>  obj-$(CONFIG_MTD_SPI_NOR)	+= spi-nor.o
>> 
>>  obj-$(CONFIG_MTD_SPI_NOR)	+= controllers/
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 80d65cfcb88d..302331695d96 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -3117,6 +3117,8 @@ static int spi_nor_probe(struct spi_mem *spimem)
>>  	if (ret)
>>  		return ret;
>> 
>> +	spi_nor_debugfs_register(nor);
> 
> I think you should register this after mtd_device_register() to be sure
> the probe would not fail with these debugfs entries still in place. Or
> clean up before returning errors in the code below. Otherwise access to
> the debugfs entires would cause you to access a nor struct that is no
> longer there.

Ohh, you are right. I'll probaly drop the unregister altogether and use
devm_add_action().

>> +
>>  	/*
>>  	 * None of the existing parts have > 512B pages, but let's play safe
>>  	 * and add this logic so that if anyone ever adds support for such
>> @@ -3148,6 +3150,8 @@ static int spi_nor_remove(struct spi_mem 
>> *spimem)
>>  {
>>  	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
>> 
>> +	spi_nor_debugfs_unregister(nor);
>> +
>>  	spi_nor_restore(nor);
>> 
>>  	/* Clean up MTD stuff. */
>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>> index 81c4bb7d3193..d042d745a1f6 100644
>> --- a/drivers/mtd/spi-nor/core.h
>> +++ b/drivers/mtd/spi-nor/core.h
>> @@ -589,4 +589,12 @@ static inline struct spi_nor 
>> *mtd_to_spi_nor(struct mtd_info *mtd)
>>  	return container_of(mtd, struct spi_nor, mtd);
>>  }
>> 
>> +#ifdef CONFIG_DEBUG_FS
>> +void spi_nor_debugfs_register(struct spi_nor *nor);
>> +void spi_nor_debugfs_unregister(struct spi_nor *nor);
>> +#else
>> +static inline void spi_nor_debugfs_register(struct spi_nor *nor) {}
>> +static inline void spi_nor_debugfs_unregister(struct spi_nor *nor) {}
>> +#endif
>> +
>>  #endif /* __LINUX_MTD_SPI_NOR_INTERNAL_H */
>> diff --git a/drivers/mtd/spi-nor/debugfs.c 
>> b/drivers/mtd/spi-nor/debugfs.c
>> new file mode 100644
>> index 000000000000..61d6d90eda13
>> --- /dev/null
>> +++ b/drivers/mtd/spi-nor/debugfs.c
>> @@ -0,0 +1,241 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include <linux/mtd/spi-nor.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/spi/spi-mem.h>
>> +#include <linux/debugfs.h>
>> +
>> +#include "core.h"
>> +
>> +static struct dentry *rootdir;
>> +
>> +#define SNOR_F_NAME(name) [ilog2(SNOR_F_##name)] = #name
>> +static const char *const snor_f_names[] = {
>> +	SNOR_F_NAME(HAS_SR_TB),
>> +	SNOR_F_NAME(NO_OP_CHIP_ERASE),
>> +	SNOR_F_NAME(BROKEN_RESET),
>> +	SNOR_F_NAME(4B_OPCODES),
>> +	SNOR_F_NAME(HAS_4BAIT),
>> +	SNOR_F_NAME(HAS_LOCK),
>> +	SNOR_F_NAME(HAS_16BIT_SR),
>> +	SNOR_F_NAME(NO_READ_CR),
>> +	SNOR_F_NAME(HAS_SR_TB_BIT6),
>> +	SNOR_F_NAME(HAS_4BIT_BP),
>> +	SNOR_F_NAME(HAS_SR_BP3_BIT6),
>> +	SNOR_F_NAME(IO_MODE_EN_VOLATILE),
>> +	SNOR_F_NAME(SOFT_RESET),
>> +	SNOR_F_NAME(SWP_IS_VOLATILE),
>> +};
>> +#undef SNOR_F_NAME
> 
> Huh, clever! Took me some time to get what this is doing. But I wonder
> if there is a way to do this so we don't have to update in 2 locations
> when adding a new flag. If not, please at least add a comment on enum
> spi_nor_option_flags to remind people to also add the flag here.

Sure can add a comment.

> 
> But I wonder if we really need this instead of doing just:
> 
> 	static const char *snor_f_names[] = {
> 		"HAS_SR_TB",
> 		"NO_OP_CHIP_ERASE",
> 		...
> 	};
> 
> Since if we have to keep this and enum spi_nor_option_flags in sync,
> might as well keep things simple.

But then the compiler wont help us catching renames and reorders.

>> +
>> +static const char *spi_nor_protocol_name(enum spi_nor_protocol proto)
>> +{
>> +	switch (proto) {
>> +	case SNOR_PROTO_1_1_1:     return "1S-1S-1S";
>> +	case SNOR_PROTO_1_1_2:     return "1S-1S-2S";
>> +	case SNOR_PROTO_1_1_4:     return "1S-1S-4S";
>> +	case SNOR_PROTO_1_1_8:     return "1S-1S-8S";
>> +	case SNOR_PROTO_1_2_2:     return "1S-2S-2S";
>> +	case SNOR_PROTO_1_4_4:     return "1S-4S-4S";
>> +	case SNOR_PROTO_1_8_8:     return "1S-8S-8S";
>> +	case SNOR_PROTO_2_2_2:     return "2S-2S-2S";
>> +	case SNOR_PROTO_4_4_4:     return "4S-4S-4S";
>> +	case SNOR_PROTO_8_8_8:     return "8S-8S-8S";
>> +	case SNOR_PROTO_1_1_1_DTR: return "1D-1D-1D";
>> +	case SNOR_PROTO_1_2_2_DTR: return "1D-2D-2D";
>> +	case SNOR_PROTO_1_4_4_DTR: return "1D-4D-4D";
>> +	case SNOR_PROTO_1_8_8_DTR: return "1D-8D-8D";
>> +	case SNOR_PROTO_8_8_8_DTR: return "8D-8D-8D";
> 
> Okay. Though this reminds me that we have no way to specify mixed DTR
> modes like 1S-8D-8D. But that is a problem for another time.
> 
>> +	}
>> +
>> +	return "<unknown>";
>> +}
>> +
>> +static void spi_nor_print_flags(struct seq_file *s, unsigned long 
>> flags,
>> +				const char *const *names, int names_len)
> 
> Do you see a need to make this generic? Why not just use snor_f_names
> directly?

There might be more flags in the future, so why should I make
this unnecessarily specific? ;)

>> +{
>> +	bool sep = false;
>> +	int i;
>> +
>> +	for (i = 0; i < sizeof(flags) * BITS_PER_BYTE; i++) {
>> +		if (!(flags & BIT(i)))
>> +			continue;
>> +		if (sep)
>> +			seq_puts(s, "|");
> 
> I have not tried running this yet, but I think putting spaces around 
> the
> '|' would make things easier to read.

Yeah and there are also no linewraps, it's just for debugging.
No need to win a beauty contest ;)

flags		HAS_16BIT_SR|NO_READ_CR|SOFT_RESET
vs
flags		HAS_16BIT_SR | NO_READ_CR | SOFT_RESET

I'm fine with either. Can add the spaces.

>> +		sep = true;
>> +		if (i < names_len && names[i])
>> +			seq_puts(s, names[i]);
>> +		else
>> +			seq_printf(s, "1<<%d", i);
> 
> Okay, so this is in case we _don't_ keep snor_f_names in sync.
> 
>> +	}
>> +}
>> +
>> +static int spi_nor_params_show(struct seq_file *s, void *data)
>> +{
>> +	struct spi_nor *nor = s->private;
>> +	struct spi_nor_flash_parameter *params = nor->params;
>> +	struct spi_nor_erase_map *erase_map = &params->erase_map;
>> +	struct spi_nor_erase_region *region;
>> +	const struct flash_info *info = nor->info;
>> +	char buf[16], *str;
>> +	int i;
>> +
>> +	seq_printf(s, "name\t\t%s\n", info->name);
>> +	seq_printf(s, "id\t\t%*phN\n", info->id_len, nor->info->id);
>> +	string_get_size(params->size, 1, STRING_UNITS_2, buf, sizeof(buf));
>> +	seq_printf(s, "size\t\t%s\n", buf);
>> +	seq_printf(s, "write size\t%u\n", params->writesize);
>> +	seq_printf(s, "page size\t%u\n", params->page_size);
>> +	seq_printf(s, "address width\t%u\n", nor->addr_width);
>> +
>> +	seq_puts(s, "flags\t\t");
>> +	spi_nor_print_flags(s, nor->flags, snor_f_names, 
>> sizeof(snor_f_names));
>> +	seq_puts(s, "\n");
>> +
>> +	seq_puts(s, "\nopcodes\n");
>> +	seq_printf(s, " read\t\t%02x\n", nor->read_opcode);
> 
> Should you prefix hex numbers with "0x" to clarify their base? If
> someone sees opcode is 12 they might not be sure if this is in decimal
> or hex. I am not sure what the convention for this usually is, if there
> even is one. Same for other hex numbers printed below.

Yeah, I wasn't sure about that either. The target audience are
developers so I just dropped the prefix (or suffix 'h'). I'll
re-add the prefix, though.

>> +	seq_printf(s, "  dummy cycles\t%d\n", nor->read_dummy);
>> +	seq_printf(s, " erase\t\t%02x\n", nor->erase_opcode);
>> +	seq_printf(s, " program\t%02x\n", nor->program_opcode);

.. esp. because the bases are mixed here.

>> +
>> +	switch (nor->cmd_ext_type) {
>> +	case SPI_NOR_EXT_NONE:
>> +		str = "none";
>> +		break;
>> +	case SPI_NOR_EXT_REPEAT:
>> +		str = "repeat";
>> +		break;
>> +	case SPI_NOR_EXT_INVERT:
>> +		str = "invert";
>> +		break;
>> +	default:
>> +		str = "<unknown>";
>> +		break;
>> +	}
>> +	seq_printf(s, " 8D extension\t%s\n", str);
> 
> You might only want to print this if the flash is 8D capable to avoid
> confusion. But I am not sure how easy/difficult that would be, so I
> leave that up to you.

This is parsed from the SFDP regardless of the mode, so it might be
helpful even if it's unused. I reckon upon the knowledge of the
developer :)

>> +
>> +	seq_puts(s, "\nprotocols\n");
>> +	seq_printf(s, " read\t\t%s\n",
>> +		   spi_nor_protocol_name(nor->read_proto));
>> +	seq_printf(s, " write\t\t%s\n",
>> +		   spi_nor_protocol_name(nor->write_proto));
>> +	seq_printf(s, " register\t%s\n",
>> +		   spi_nor_protocol_name(nor->reg_proto));
>> +
>> +	seq_puts(s, "\nerase commands\n");
>> +	for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++) {
>> +		struct spi_nor_erase_type *et = &erase_map->erase_type[i];
>> +
>> +		if (et->size) {
>> +			string_get_size(et->size, 1, STRING_UNITS_2, buf,
>> +					sizeof(buf));
>> +			seq_printf(s, " %02x (%s) [%d]\n", et->opcode, buf, i);
>> +		}
>> +	}
>> +
>> +	if (!(nor->flags & SNOR_F_NO_OP_CHIP_ERASE)) {
>> +		string_get_size(params->size, 1, STRING_UNITS_2, buf, sizeof(buf));
>> +		seq_printf(s, " %02x (%s)\n", SPINOR_OP_CHIP_ERASE, buf);
>> +	}
>> +
>> +	seq_puts(s, "\nsector map\n");
>> +	seq_puts(s, " region              | erase mask | flags\n");
>> +	seq_puts(s, " --------------------+------------+----------\n");
>> +	for (region = erase_map->regions;
>> +	     region;
>> +	     region = spi_nor_region_next(region)) {
>> +		u64 start = region->offset & ~SNOR_ERASE_FLAGS_MASK;
>> +		u64 flags = region->offset & SNOR_ERASE_FLAGS_MASK;
>> +		u64 end = start + region->size - 1;
>> +
>> +		seq_printf(s, " %08llx - %08llx |     [%c%c%c%c] | %s\n",
>> +			   start, end,
>> +			   flags & BIT(0) ? '0' : ' ',
>> +			   flags & BIT(1) ? '1' : ' ',
>> +			   flags & BIT(2) ? '2' : ' ',
>> +			   flags & BIT(3) ? '3' : ' ',
>> +			   flags & SNOR_OVERLAID_REGION ? "overlaid" : "");
>> +	}
>> +
>> +	return 0;
>> +}
>> +DEFINE_SHOW_ATTRIBUTE(spi_nor_params);
>> +
>> +static void spi_nor_print_read_cmd(struct seq_file *s, u32 cap,
>> +				   struct spi_nor_read_command *cmd)
>> +{
>> +	seq_printf(s, "%s%s\n", spi_nor_protocol_name(cmd->proto),
>> +		   cap == SNOR_HWCAPS_READ_FAST ? " (fast read)" : "");
>> +	seq_printf(s, "  opcode\t%02x\n", cmd->opcode);
>> +	seq_printf(s, "  mode cycles\t%02x\n", cmd->num_mode_clocks);
>> +	seq_printf(s, "  dummy cycles\t%02x\n", cmd->num_wait_states);
>> +}
>> +
>> +static void spi_nor_print_pp_cmd(struct seq_file *s,
>> +				 struct spi_nor_pp_command *cmd)
>> +{
>> +	seq_printf(s, "%s\n", spi_nor_protocol_name(cmd->proto));
>> +	seq_printf(s, "  opcode\t%02x\n", cmd->opcode);
>> +}
>> +
>> +static int spi_nor_capabilities_show(struct seq_file *s, void *data)
>> +{
>> +	struct spi_nor *nor = s->private;
>> +	struct spi_nor_flash_parameter *params = nor->params;
>> +	u32 hwcaps = params->hwcaps.mask;
>> +	int i, cmd;
>> +
>> +	seq_puts(s, "Supported read modes by the flash\n");
>> +	for (i = 0; i < sizeof(hwcaps) * BITS_PER_BYTE; i++) {
>> +		if (!(hwcaps & BIT(i)))
>> +			continue;
>> +
>> +		cmd = spi_nor_hwcaps_read2cmd(BIT(i));
>> +		if (cmd < 0)
>> +			continue;
>> +
>> +		spi_nor_print_read_cmd(s, BIT(i), &params->reads[cmd]);
>> +		hwcaps &= ~BIT(i);
>> +	}
>> +
>> +	seq_puts(s, "\nSupported page program modes by the flash\n");
>> +	for (i = 0; i < sizeof(hwcaps) * BITS_PER_BYTE; i++) {
>> +		if (!(hwcaps & BIT(i)))
>> +			continue;
>> +
>> +		cmd = spi_nor_hwcaps_pp2cmd(BIT(i));
>> +		if (cmd < 0)
>> +			continue;
>> +
>> +		spi_nor_print_pp_cmd(s, &params->page_programs[cmd]);
>> +		hwcaps &= ~BIT(i);
>> +	}
>> +
>> +	if (hwcaps)
>> +		seq_printf(s, "\nunknown hwcaps %x\n", hwcaps);
>> +
>> +	return 0;
>> +}
>> +DEFINE_SHOW_ATTRIBUTE(spi_nor_capabilities);
>> +
>> +void spi_nor_debugfs_register(struct spi_nor *nor)
>> +{
>> +	struct dentry *d;
>> +
>> +	/* Create rootdir once. Will never be deleted again. */
>> +	if (!rootdir)
>> +		rootdir = debugfs_create_dir("spi-nor", NULL);
>> +
>> +	d = debugfs_create_dir(dev_name(nor->dev), rootdir);
>> +	nor->debugfs_root = d;
>> +
>> +	debugfs_create_file("params", 0444, d, nor, &spi_nor_params_fops);
>> +	debugfs_create_file("capabilities", 0444, d, nor,
>> +			    &spi_nor_capabilities_fops);
>> +}
>> +
>> +void spi_nor_debugfs_unregister(struct spi_nor *nor)
>> +{
>> +	debugfs_remove(nor->debugfs_root);
>> +	nor->debugfs_root = NULL;
>> +}
>> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
>> index 5e25a7b75ae2..7d43447768ee 100644
>> --- a/include/linux/mtd/spi-nor.h
>> +++ b/include/linux/mtd/spi-nor.h
>> @@ -365,6 +365,7 @@ struct spi_nor_flash_parameter;
>>   * @write_proto:	the SPI protocol for write operations
>>   * @reg_proto:		the SPI protocol for read_reg/write_reg/erase 
>> operations
>>   * @sfdp:		the SFDP data of the flash
>> + * @debugfs_root:	pointer to the debugfs directory
>>   * @controller_ops:	SPI NOR controller driver specific operations.
>>   * @params:		[FLASH-SPECIFIC] SPI NOR flash parameters and settings.
>>   *                      The structure includes legacy flash 
>> parameters and
>> @@ -394,6 +395,7 @@ struct spi_nor {
>>  	u32			flags;
>>  	enum spi_nor_cmd_ext	cmd_ext_type;
>>  	struct sfdp		*sfdp;
>> +	struct dentry		*debugfs_root;
>> 
>>  	const struct spi_nor_controller_ops *controller_ops;
>> 
>> --
>> 2.30.2
>> 

-- 
-michael

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

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-04-20  9:18 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-18 12:10 [PATCH RFC v1 1/2] mtd: spi-nor: export spi_nor_hwcaps_pp2cmd() Michael Walle
2022-04-18 12:10 ` Michael Walle
2022-04-18 12:10 ` [PATCH RFC v1 2/2] mtd: spi-nor: expose internal parameters via debugfs Michael Walle
2022-04-18 12:10   ` Michael Walle
2022-04-18 12:13   ` Michael Walle
2022-04-18 12:13     ` Michael Walle
2022-04-20  5:23   ` Pratyush Yadav
2022-04-20  5:23     ` Pratyush Yadav
2022-04-20  9:18     ` Michael Walle
2022-04-20  9:18       ` Michael Walle

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.