diff mbox series

[v5,1/3] debugfs_create_regset32() support 8/16 bit width registers

Message ID 20231002183750.552759-2-Frank.Li@nxp.com (mailing list archive)
State Superseded
Headers show
Series dmaengine: fsl_edma: add trace and debugfs support | expand

Commit Message

Frank Li Oct. 2, 2023, 6:37 p.m. UTC 
Enhance the flexibility of `debugfs_create_regset32()` to support registers
of various bit widths. The key changes are as follows:

1. Renamed '*reg32' and '*regset32' to '*reg' and '*regset' in relevant
   code to reflect that the register width is not limited to 32 bits.

2. Added 'size' and 'bigendian' fields to the `struct debugfs_reg` to allow
   for specifying the size and endianness of registers. These additions
   enable `debugfs_create_regset()` to support a wider range of register
   types.

3. When 'size' is set to 0, it signifies a 32-bit register. This change
   maintains compatibility with existing code that assumes 32-bit
   registers.

Improve the versatility of `debugfs_create_regset()` and enable it to
handle registers of different sizes and endianness, offering greater
flexibility for debugging and monitoring.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 fs/debugfs/file.c       | 53 +++++++++++++++++++++++++++--------------
 include/linux/debugfs.h | 17 +++++++++----
 2 files changed, 48 insertions(+), 22 deletions(-)

Comments

Arnd Bergmann Oct. 2, 2023, 6:55 p.m. UTC | #1 
On Mon, Oct 2, 2023, at 20:37, Frank Li wrote:
> Enhance the flexibility of `debugfs_create_regset32()` to support registers
> of various bit widths. The key changes are as follows:
>
> 1. Renamed '*reg32' and '*regset32' to '*reg' and '*regset' in relevant
>    code to reflect that the register width is not limited to 32 bits.
>
> 2. Added 'size' and 'bigendian' fields to the `struct debugfs_reg` to allow
>    for specifying the size and endianness of registers. These additions
>    enable `debugfs_create_regset()` to support a wider range of register
>    types.
>
> 3. When 'size' is set to 0, it signifies a 32-bit register. This change
>    maintains compatibility with existing code that assumes 32-bit
>    registers.
>
> Improve the versatility of `debugfs_create_regset()` and enable it to
> handle registers of different sizes and endianness, offering greater
> flexibility for debugging and monitoring.
>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>

This version looks correct to me, I see no fundamental problems with it.
In fact, you could list "support for ioport_map() output" to the features
above.

A few other thoughts from my side, all of which could be ignored:

- if the ioport access is not an important feature, we can instead
  support 64-bit readl() as I commented in a previous email. We just
  can't easily have both.

- instead of treating every value of "regs->size" other than 1 and 2
  as meaning '32-bit read', I would explicitly check for 0 and 4
  here

- Another more complicated but also more featureful variant would
  be to use the 'regmap' infrastructure as the abstraction, this would
  also provide access to big-endian, variable register width
  (including 64-bit), and pio, along with additional features and
  other bus types. Not sure it's worth it, but could be interesting
  to try out.

     Arnd
Frank Li Oct. 2, 2023, 7:07 p.m. UTC | #2 
On Mon, Oct 02, 2023 at 08:55:23PM +0200, Arnd Bergmann wrote:
> On Mon, Oct 2, 2023, at 20:37, Frank Li wrote:
> > Enhance the flexibility of `debugfs_create_regset32()` to support registers
> > of various bit widths. The key changes are as follows:
> >
> > 1. Renamed '*reg32' and '*regset32' to '*reg' and '*regset' in relevant
> >    code to reflect that the register width is not limited to 32 bits.
> >
> > 2. Added 'size' and 'bigendian' fields to the `struct debugfs_reg` to allow
> >    for specifying the size and endianness of registers. These additions
> >    enable `debugfs_create_regset()` to support a wider range of register
> >    types.
> >
> > 3. When 'size' is set to 0, it signifies a 32-bit register. This change
> >    maintains compatibility with existing code that assumes 32-bit
> >    registers.
> >
> > Improve the versatility of `debugfs_create_regset()` and enable it to
> > handle registers of different sizes and endianness, offering greater
> > flexibility for debugging and monitoring.
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> 
> This version looks correct to me, I see no fundamental problems with it.
> In fact, you could list "support for ioport_map() output" to the features
> above.
> 
> A few other thoughts from my side, all of which could be ignored:
> 
> - if the ioport access is not an important feature, we can instead
>   support 64-bit readl() as I commented in a previous email. We just
>   can't easily have both.

We will get 64bit dma edma soon. So I can test and upstream it when I get
it.

> 
> - instead of treating every value of "regs->size" other than 1 and 2
>   as meaning '32-bit read', I would explicitly check for 0 and 4
>   here

Yes, I will update next version.

> 
> - Another more complicated but also more featureful variant would
>   be to use the 'regmap' infrastructure as the abstraction, this would
>   also provide access to big-endian, variable register width
>   (including 64-bit), and pio, along with additional features and
>   other bus types. Not sure it's worth it, but could be interesting
>   to try out.

Yes, debugfs_create_regset may need big change or create new version for
regmap. It is out of scope this patches.  I will try later.

> 
>      Arnd
Arnd Bergmann Oct. 2, 2023, 7:39 p.m. UTC | #3 
On Mon, Oct 2, 2023, at 21:07, Frank Li wrote:
> On Mon, Oct 02, 2023 at 08:55:23PM +0200, Arnd Bergmann wrote:
>
>> A few other thoughts from my side, all of which could be ignored:
>> 
>> - if the ioport access is not an important feature, we can instead
>>   support 64-bit readl() as I commented in a previous email. We just
>>   can't easily have both.
>
> We will get 64bit dma edma soon. So I can test and upstream it when I get
> it.

Ok, so if we already know this is going to be needed, then I would skip
the PIO support and just use read{bwlq}() with the optional swab() instead
of the ioread variants. Otherwise there is a risk that someone starts
relying on the port I/O feature and make it harder to remove.

     Arnd
diff mbox series

Patch

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 87b3753aa4b1..62cc96bb6d72 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -1137,15 +1137,15 @@  EXPORT_SYMBOL_GPL(debugfs_create_u32_array);
 #ifdef CONFIG_HAS_IOMEM
 
 /*
- * The regset32 stuff is used to print 32-bit registers using the
+ * The regset stuff is used to print 32-bit registers using the
  * seq_file utilities. We offer printing a register set in an already-opened
- * sequential file or create a debugfs file that only prints a regset32.
+ * sequential file or create a debugfs file that only prints a regset.
  */
 
 /**
- * debugfs_print_regs32 - use seq_print to describe a set of registers
+ * debugfs_print_regs - use seq_print to describe a set of registers
  * @s: the seq_file structure being used to generate output
- * @regs: an array if struct debugfs_reg32 structures
+ * @regs: an array if struct debugfs_reg structures
  * @nregs: the length of the above array
  * @base: the base address to be used in reading the registers
  * @prefix: a string to be prefixed to every output line
@@ -1157,30 +1157,47 @@  EXPORT_SYMBOL_GPL(debugfs_create_u32_array);
  * because some peripherals have several blocks of identical registers,
  * for example configuration of dma channels
  */
-void debugfs_print_regs32(struct seq_file *s, const struct debugfs_reg32 *regs,
+void debugfs_print_regs(struct seq_file *s, const struct debugfs_reg *regs,
 			  int nregs, void __iomem *base, char *prefix)
 {
+	void __iomem *reg;
+	bool b;
 	int i;
 
 	for (i = 0; i < nregs; i++, regs++) {
 		if (prefix)
 			seq_printf(s, "%s", prefix);
-		seq_printf(s, "%s = 0x%08x\n", regs->name,
-			   readl(base + regs->offset));
+
+		b = regs->bigendian;
+		reg = base + regs->offset;
+
+		switch (regs->size) {
+		case sizeof(u8):
+			seq_printf(s, "%s = 0x%02x\n", regs->name, ioread8(reg));
+			break;
+		case sizeof(u16):
+			seq_printf(s, "%s = 0x%04x\n", regs->name,
+				  b ? ioread16be(reg) : ioread16(reg));
+			break;
+		default:
+			seq_printf(s, "%s = 0x%08x\n", regs->name,
+				   b ? ioread32be(reg) : ioread32(reg));
+		}
+
 		if (seq_has_overflowed(s))
 			break;
 	}
 }
-EXPORT_SYMBOL_GPL(debugfs_print_regs32);
+EXPORT_SYMBOL_GPL(debugfs_print_regs);
 
-static int debugfs_regset32_show(struct seq_file *s, void *data)
+static int debugfs_regset_show(struct seq_file *s, void *data)
 {
-	struct debugfs_regset32 *regset = s->private;
+	struct debugfs_regset *regset = s->private;
 
 	if (regset->dev)
 		pm_runtime_get_sync(regset->dev);
 
-	debugfs_print_regs32(s, regset->regs, regset->nregs, regset->base, "");
+	debugfs_print_regs(s, regset->regs, regset->nregs, regset->base, "");
 
 	if (regset->dev)
 		pm_runtime_put(regset->dev);
@@ -1188,16 +1205,16 @@  static int debugfs_regset32_show(struct seq_file *s, void *data)
 	return 0;
 }
 
-DEFINE_SHOW_ATTRIBUTE(debugfs_regset32);
+DEFINE_SHOW_ATTRIBUTE(debugfs_regset);
 
 /**
- * debugfs_create_regset32 - create a debugfs file that returns register values
+ * debugfs_create_regset - create a debugfs file that returns register values
  * @name: a pointer to a string containing the name of the file to create.
  * @mode: the permission that the file should have
  * @parent: a pointer to the parent dentry for this file.  This should be a
  *          directory dentry if set.  If this parameter is %NULL, then the
  *          file will be created in the root of the debugfs filesystem.
- * @regset: a pointer to a struct debugfs_regset32, which contains a pointer
+ * @regset: a pointer to a struct debugfs_regset, which contains a pointer
  *          to an array of register definitions, the array size and the base
  *          address where the register bank is to be found.
  *
@@ -1205,13 +1222,13 @@  DEFINE_SHOW_ATTRIBUTE(debugfs_regset32);
  * the names and values of a set of 32-bit registers. If the @mode variable
  * is so set it can be read from. Writing is not supported.
  */
-void debugfs_create_regset32(const char *name, umode_t mode,
+void debugfs_create_regset(const char *name, umode_t mode,
 			     struct dentry *parent,
-			     struct debugfs_regset32 *regset)
+			     struct debugfs_regset *regset)
 {
-	debugfs_create_file(name, mode, parent, regset, &debugfs_regset32_fops);
+	debugfs_create_file(name, mode, parent, regset, &debugfs_regset_fops);
 }
-EXPORT_SYMBOL_GPL(debugfs_create_regset32);
+EXPORT_SYMBOL_GPL(debugfs_create_regset);
 
 #endif /* CONFIG_HAS_IOMEM */
 
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index ea2d919fd9c7..247ae4217ea5 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -26,18 +26,24 @@  struct debugfs_blob_wrapper {
 	unsigned long size;
 };
 
-struct debugfs_reg32 {
+struct debugfs_reg {
 	char *name;
+	int size;
+	int bigendian;
 	unsigned long offset;
 };
 
-struct debugfs_regset32 {
+#define debugfs_reg32 debugfs_reg
+
+struct debugfs_regset {
 	const struct debugfs_reg32 *regs;
 	int nregs;
 	void __iomem *base;
 	struct device *dev;	/* Optional device for Runtime PM */
 };
 
+#define debugfs_regset32 debugfs_regset
+
 struct debugfs_u32_array {
 	u32 *array;
 	u32 n_elements;
@@ -145,12 +151,15 @@  struct dentry *debugfs_create_blob(const char *name, umode_t mode,
 				  struct dentry *parent,
 				  struct debugfs_blob_wrapper *blob);
 
-void debugfs_create_regset32(const char *name, umode_t mode,
+void debugfs_create_regset(const char *name, umode_t mode,
 			     struct dentry *parent,
 			     struct debugfs_regset32 *regset);
 
-void debugfs_print_regs32(struct seq_file *s, const struct debugfs_reg32 *regs,
+#define debugfs_create_regset32 debugfs_create_regset
+
+void debugfs_print_regs(struct seq_file *s, const struct debugfs_reg32 *regs,
 			  int nregs, void __iomem *base, char *prefix);
+#define debugfs_print_regs32 debugfs_print_regs
 
 void debugfs_create_u32_array(const char *name, umode_t mode,
 			      struct dentry *parent,