Hey Basavaraj,
thanks for the rework. From what I can see most looks quite good now.
Just found one more thing; sorry for not providing it sooner:
On Mon, 2024-07-01 at 22:12 +0530, Basavaraj Natikar wrote:
> To support multi-channel functionality with AE4DMA engine, extend the
> ptdma-debugfs with reusable components.
>
> Reviewed-by: Raju Rangoju <Raju.Rangoju@xxxxxxx>
> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@xxxxxxx>
> ---
> drivers/dma/amd/ptdma/ptdma-debugfs.c | 76 +++++++++++++++++++------
> --
> 1 file changed, 55 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/dma/amd/ptdma/ptdma-debugfs.c
> b/drivers/dma/amd/ptdma/ptdma-debugfs.c
> index c8307d3044a3..9aa7a49ae5be 100644
> --- a/drivers/dma/amd/ptdma/ptdma-debugfs.c
> +++ b/drivers/dma/amd/ptdma/ptdma-debugfs.c
> @@ -12,7 +12,7 @@
> #include <linux/debugfs.h>
> #include <linux/seq_file.h>
>
> -#include "ptdma.h"
> +#include "../common/amd_dma.h"
>
> /* DebugFS helpers */
> #define RI_VERSION_NUM 0x0000003F
> @@ -23,11 +23,19 @@
> static int pt_debugfs_info_show(struct seq_file *s, void *p)
> {
> struct pt_device *pt = s->private;
> + struct ae4_device *ae4;
> unsigned int regval;
>
> seq_printf(s, "Device name: %s\n", dev_name(pt->dev));
> - seq_printf(s, " # Queues: %d\n", 1);
> - seq_printf(s, " # Cmds: %d\n", pt->cmd_count);
> +
> + if (pt->ver == AE4_DMA_VERSION) {
> + ae4 = container_of(pt, struct ae4_device, pt);
> + seq_printf(s, " # Queues: %d\n", ae4->cmd_q_count);
> + seq_printf(s, " # Cmds per queue: %d\n",
> CMD_Q_LEN);
> + } else {
> + seq_printf(s, " # Queues: %d\n", 1);
> + seq_printf(s, " # Cmds: %d\n", pt->cmd_count);
> + }
>
> regval = ioread32(pt->io_regs + CMD_PT_VERSION);
>
> @@ -55,6 +63,7 @@ static int pt_debugfs_stats_show(struct seq_file
> *s, void *p)
> static int pt_debugfs_queue_show(struct seq_file *s, void *p)
> {
> struct pt_cmd_queue *cmd_q = s->private;
> + struct pt_device *pt;
> unsigned int regval;
>
> if (!cmd_q)
> @@ -62,18 +71,24 @@ static int pt_debugfs_queue_show(struct seq_file
> *s, void *p)
>
> seq_printf(s, " Pass-Thru: %ld\n", cmd_q-
> >total_pt_ops);
>
> - regval = ioread32(cmd_q->reg_control + 0x000C);
> -
> - seq_puts(s, " Enabled Interrupts:");
> - if (regval & INT_EMPTY_QUEUE)
> - seq_puts(s, " EMPTY");
> - if (regval & INT_QUEUE_STOPPED)
> - seq_puts(s, " STOPPED");
> - if (regval & INT_ERROR)
> - seq_puts(s, " ERROR");
> - if (regval & INT_COMPLETION)
> - seq_puts(s, " COMPLETION");
> - seq_puts(s, "\n");
> + pt = cmd_q->pt;
> + if (pt->ver == AE4_DMA_VERSION) {
> + regval = readl(cmd_q->reg_control + 0x4);
> + seq_printf(s, " Enabled Interrupts:: status
> 0x%x\n", regval);
> + } else {
> + regval = ioread32(cmd_q->reg_control + 0x000C);
> +
> + seq_puts(s, " Enabled Interrupts:");
> + if (regval & INT_EMPTY_QUEUE)
> + seq_puts(s, " EMPTY");
> + if (regval & INT_QUEUE_STOPPED)
> + seq_puts(s, " STOPPED");
> + if (regval & INT_ERROR)
> + seq_puts(s, " ERROR");
> + if (regval & INT_COMPLETION)
> + seq_puts(s, " COMPLETION");
> + seq_puts(s, "\n");
> + }
>
> return 0;
> }
> @@ -84,8 +99,12 @@ DEFINE_SHOW_ATTRIBUTE(pt_debugfs_stats);
>
> void ptdma_debugfs_setup(struct pt_device *pt)
> {
> - struct pt_cmd_queue *cmd_q;
> struct dentry *debugfs_q_instance;
> + struct ae4_cmd_queue *ae4cmd_q;
> + struct pt_cmd_queue *cmd_q;
> + struct ae4_device *ae4;
> + char name[30];
Constant _might_ be better unless you're sure that the function will
seldomly be changed, and:
> + int i;
>
> if (!debugfs_initialized())
> return;
> @@ -96,11 +115,26 @@ void ptdma_debugfs_setup(struct pt_device *pt)
> debugfs_create_file("stats", 0400, pt->dma_dev.dbg_dev_root,
> pt,
> &pt_debugfs_stats_fops);
>
> - cmd_q = &pt->cmd_q;
>
> - debugfs_q_instance =
> - debugfs_create_dir("q", pt->dma_dev.dbg_dev_root);
> + if (pt->ver == AE4_DMA_VERSION) {
> + ae4 = container_of(pt, struct ae4_device, pt);
> + for (i = 0; i < ae4->cmd_q_count; i++) {
> + ae4cmd_q = &ae4->ae4cmd_q[i];
> + cmd_q = &ae4cmd_q->cmd_q;
> +
> + snprintf(name, 29, "q%d", ae4cmd_q->id);
That seems wrong to me. The trailing '\0' should be included:
/**
* snprintf - Format a string and place it in a buffer
* @buf: The buffer to place the result into
* @size: The size of the buffer, including the trailing null space
* @fmt: The format string to use
* @...: Arguments for the format string
*
* The return value is the number of characters which would be
* generated for the given input, excluding the trailing null,
* as per ISO C99. If the return is greater than or equal to
* @size, the resulting string is truncated.
*
* See the vsnprintf() documentation for format string extensions over C99.
*/
int snprintf(char *buf, size_t size, const char *fmt, ...)
Greetings,
P.
> +
> + debugfs_q_instance =
> + debugfs_create_dir(name, pt-
> >dma_dev.dbg_dev_root);
>
> - debugfs_create_file("stats", 0400, debugfs_q_instance, cmd_q,
> - &pt_debugfs_queue_fops);
> + debugfs_create_file("stats", 0400,
> debugfs_q_instance, cmd_q,
> + &pt_debugfs_queue_fops);
> + }
> + } else {
> + debugfs_q_instance =
> + debugfs_create_dir("q", pt-
> >dma_dev.dbg_dev_root);
> + cmd_q = &pt->cmd_q;
> + debugfs_create_file("stats", 0400,
> debugfs_q_instance, cmd_q,
> + &pt_debugfs_queue_fops);
> + }
> }
