[05/16] remoteproc/pru: Add pru-specific debugfs support
diff mbox series

Message ID 1543218769-5507-6-git-send-email-rogerq@ti.com
State New
Headers show
Series
  • remoteproc: Add support for TI PRU
Related show

Commit Message

Roger Quadros Nov. 26, 2018, 7:52 a.m. UTC
From: Suman Anna <s-anna@ti.com>

The remoteproc core creates certain standard debugfs entries,
that does not give a whole lot of useful information for the
PRUs. The PRU remoteproc driver is enhanced to add additional
debugfs entries for PRU. These will be auto-cleaned up when
the parent rproc debug directory is removed.

The enhanced debugfs support adds two new entries: 'regs' and
'single_step'. The 'regs' dumps out the useful CTRL sub-module
registers as well as each of the 32 GPREGs and CT_REGs registers.
The GPREGs and CT_REGs though are printed only when the PRU is
halted and accessible as per the IP design.

The 'single_step' utilizes the single-step execution of the PRU
cores. Writing a non-zero value performs a single step, and a
zero value restores the PRU to execute in the same mode as the
mode before the first single step. (note: if the PRU is halted
because of a halt instruction, then no change occurs).

Logic for setting the PC and jumping over a halt instruction shall
be added in the future.

Signed-off-by: Suman Anna <s-anna@ti.com>
---
 drivers/remoteproc/pru_rproc.c | 135 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 135 insertions(+)

Comments

David Lechner Nov. 26, 2018, 10:37 p.m. UTC | #1
On 11/26/18 1:52 AM, Roger Quadros wrote:
> From: Suman Anna <s-anna@ti.com>
> 
> The remoteproc core creates certain standard debugfs entries,
> that does not give a whole lot of useful information for the
> PRUs. The PRU remoteproc driver is enhanced to add additional
> debugfs entries for PRU. These will be auto-cleaned up when
> the parent rproc debug directory is removed.
> 
> The enhanced debugfs support adds two new entries: 'regs' and
> 'single_step'. The 'regs' dumps out the useful CTRL sub-module
> registers as well as each of the 32 GPREGs and CT_REGs registers.
> The GPREGs and CT_REGs though are printed only when the PRU is
> halted and accessible as per the IP design.
> 

If the driver used regmap to access the CTRL I/O memory, then
'regs' wouldn't be needed since regmap already does debugfs.
Roger Quadros Nov. 29, 2018, 10:17 a.m. UTC | #2
On 27/11/18 00:37, David Lechner wrote:
> On 11/26/18 1:52 AM, Roger Quadros wrote:
>> From: Suman Anna <s-anna@ti.com>
>>
>> The remoteproc core creates certain standard debugfs entries,
>> that does not give a whole lot of useful information for the
>> PRUs. The PRU remoteproc driver is enhanced to add additional
>> debugfs entries for PRU. These will be auto-cleaned up when
>> the parent rproc debug directory is removed.
>>
>> The enhanced debugfs support adds two new entries: 'regs' and
>> 'single_step'. The 'regs' dumps out the useful CTRL sub-module
>> registers as well as each of the 32 GPREGs and CT_REGs registers.
>> The GPREGs and CT_REGs though are printed only when the PRU is
>> halted and accessible as per the IP design.
>>
> 
> If the driver used regmap to access the CTRL I/O memory, then
> 'regs' wouldn't be needed since regmap already does debugfs.
> 

ok, we could split out CTRL from this and use regmap.

cheers,
-roger
Roger Quadros Dec. 18, 2018, 3:51 p.m. UTC | #3
+Mark

David,

On 29/11/18 12:17, Roger Quadros wrote:
> On 27/11/18 00:37, David Lechner wrote:
>> On 11/26/18 1:52 AM, Roger Quadros wrote:
>>> From: Suman Anna <s-anna@ti.com>
>>>
>>> The remoteproc core creates certain standard debugfs entries,
>>> that does not give a whole lot of useful information for the
>>> PRUs. The PRU remoteproc driver is enhanced to add additional
>>> debugfs entries for PRU. These will be auto-cleaned up when
>>> the parent rproc debug directory is removed.
>>>
>>> The enhanced debugfs support adds two new entries: 'regs' and
>>> 'single_step'. The 'regs' dumps out the useful CTRL sub-module
>>> registers as well as each of the 32 GPREGs and CT_REGs registers.
>>> The GPREGs and CT_REGs though are printed only when the PRU is
>>> halted and accessible as per the IP design.
>>>
>>
>> If the driver used regmap to access the CTRL I/O memory, then
>> 'regs' wouldn't be needed since regmap already does debugfs.
>>
> 
> ok, we could split out CTRL from this and use regmap.
> 

I was thinking of using regmap for both control and debug
but regmap_mmio only supports one iomap even though it supports
multiple ranges.

What can we do here?

The control and debug regions even though separate are right
next to each other

reg = <0x34000 0x3000>,
	<0x22000 0x400>,
	<0x22400 0x100>;
reg-names = "iram", "control", "debug";

We could combine control and debug into one iomap and use
2 regmap ranges. But this is really working around the
regmap_mmio limitation of not being able to use more than one ioremaps.

Mark, any suggestions?

Is it meaningful to support __devm_regmap_init_mmio_clk()
with separate iomem regions for the ranges?

cheers,
-roger
Mark Brown Dec. 19, 2018, 12:38 p.m. UTC | #4
On Tue, Dec 18, 2018 at 05:51:12PM +0200, Roger Quadros wrote:

> We could combine control and debug into one iomap and use
> 2 regmap ranges. But this is really working around the
> regmap_mmio limitation of not being able to use more than one ioremaps.

> Mark, any suggestions?

If they're separate regions why not create separate regmaps for them?
Roger Quadros Dec. 19, 2018, 3:43 p.m. UTC | #5
On 19/12/18 14:38, Mark Brown wrote:
> On Tue, Dec 18, 2018 at 05:51:12PM +0200, Roger Quadros wrote:
> 
>> We could combine control and debug into one iomap and use
>> 2 regmap ranges. But this is really working around the
>> regmap_mmio limitation of not being able to use more than one ioremaps.
> 
>> Mark, any suggestions?
> 
> If they're separate regions why not create separate regmaps for them?
> 

I tried that but only the first regmap shows up in debugfs.

e.g.

+
+       pru->ctrl_regmap = devm_regmap_init_mmio(dev, pru->iomem_regions[PRU_IOMEM_CTRL].va,
+                                                &pru_regmap_config);
+       if (IS_ERR(pru->ctrl_regmap)) {
+               ret = PTR_ERR(pru->ctrl_regmap);
+               dev_err(dev, "CTRL regmap init failed: %d\n", ret);
+               goto free_rproc;
+       }
+
+
+       pru->debug_regmap = devm_regmap_init_mmio(dev, pru->iomem_regions[PRU_IOMEM_DEBUG].va,
+                                                 &pru_debug_regmap_config);
+       if (IS_ERR(pru->debug_regmap)) {
+               ret = PTR_ERR(pru->debug_regmap);
+               dev_err(dev, "DEBUG regmap init failed: %d\n", ret);
+               goto free_rproc;
        }


Did I do something wrong or we just need to enhance regmap_debugfs.c?

cheers,
-roger
David Lechner Dec. 19, 2018, 3:48 p.m. UTC | #6
On 12/19/18 4:43 PM, Roger Quadros wrote:
> On 19/12/18 14:38, Mark Brown wrote:
>> On Tue, Dec 18, 2018 at 05:51:12PM +0200, Roger Quadros wrote:
>>
>>> We could combine control and debug into one iomap and use
>>> 2 regmap ranges. But this is really working around the
>>> regmap_mmio limitation of not being able to use more than one ioremaps.
>>
>>> Mark, any suggestions?
>>
>> If they're separate regions why not create separate regmaps for them?
>>
> 
> I tried that but only the first regmap shows up in debugfs.
> 
> e.g.
> 
> +
> +       pru->ctrl_regmap = devm_regmap_init_mmio(dev, pru->iomem_regions[PRU_IOMEM_CTRL].va,
> +                                                &pru_regmap_config);
> +       if (IS_ERR(pru->ctrl_regmap)) {
> +               ret = PTR_ERR(pru->ctrl_regmap);
> +               dev_err(dev, "CTRL regmap init failed: %d\n", ret);
> +               goto free_rproc;
> +       }
> +
> +
> +       pru->debug_regmap = devm_regmap_init_mmio(dev, pru->iomem_regions[PRU_IOMEM_DEBUG].va,
> +                                                 &pru_debug_regmap_config);
> +       if (IS_ERR(pru->debug_regmap)) {
> +               ret = PTR_ERR(pru->debug_regmap);
> +               dev_err(dev, "DEBUG regmap init failed: %d\n", ret);
> +               goto free_rproc;
>          }
> 
> 
> Did I do something wrong or we just need to enhance regmap_debugfs.c?

Do you assign the name field in pru_regmap_config and 
pru_debug_regmap_config?
Mark Brown Dec. 19, 2018, 5:07 p.m. UTC | #7
On Wed, Dec 19, 2018 at 04:48:52PM +0100, David Lechner wrote:
> On 12/19/18 4:43 PM, Roger Quadros wrote:

> > Did I do something wrong or we just need to enhance regmap_debugfs.c?

> Do you assign the name field in pru_regmap_config and
> pru_debug_regmap_config?

Yeah, you'll at least need to override the name since the default is to
use the name of the device and that'll result in two duplicates.
Tony Lindgren Dec. 19, 2018, 5:18 p.m. UTC | #8
* Mark Brown <broonie@kernel.org> [181219 17:07]:
> On Wed, Dec 19, 2018 at 04:48:52PM +0100, David Lechner wrote:
> > On 12/19/18 4:43 PM, Roger Quadros wrote:
> 
> > > Did I do something wrong or we just need to enhance regmap_debugfs.c?
> 
> > Do you assign the name field in pru_regmap_config and
> > pru_debug_regmap_config?
> 
> Yeah, you'll at least need to override the name since the default is to
> use the name of the device and that'll result in two duplicates.

Also combining IO regions often has a serious issue that the
separate IO regions may not be in the same device (or same
interconnect target module).

This means that flushing a posted write with a read-back for
an IO region will only flush it for a single device and not
the other(s). And this leads into hard to find bugs :)

Regards,

Tony
Roger Quadros Dec. 20, 2018, 8:45 a.m. UTC | #9
On 19/12/18 19:18, Tony Lindgren wrote:
> * Mark Brown <broonie@kernel.org> [181219 17:07]:
>> On Wed, Dec 19, 2018 at 04:48:52PM +0100, David Lechner wrote:
>>> On 12/19/18 4:43 PM, Roger Quadros wrote:
>>
>>>> Did I do something wrong or we just need to enhance regmap_debugfs.c?
>>
>>> Do you assign the name field in pru_regmap_config and
>>> pru_debug_regmap_config?
>>
>> Yeah, you'll at least need to override the name since the default is to
>> use the name of the device and that'll result in two duplicates.

I hadn't set the name field and this was the issue. It is resolved now.
Thanks :)

> 
> Also combining IO regions often has a serious issue that the
> separate IO regions may not be in the same device (or same
> interconnect target module).
> 
> This means that flushing a posted write with a read-back for
> an IO region will only flush it for a single device and not
> the other(s). And this leads into hard to find bugs :)

Agree.

cheers,
-roger

Patch
diff mbox series

diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
index c35f432..73a7f13 100644
--- a/drivers/remoteproc/pru_rproc.c
+++ b/drivers/remoteproc/pru_rproc.c
@@ -8,6 +8,7 @@ 
  */
 
 #include <linux/bitops.h>
+#include <linux/debugfs.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
@@ -36,6 +37,10 @@ 
 #define CTRL_CTRL_SINGLE_STEP	BIT(8)
 #define CTRL_CTRL_RUNSTATE	BIT(15)
 
+/* PRU_ICSS_PRU_DEBUG registers */
+#define PRU_DEBUG_GPREG(x)	(0x0000 + (x) * 4)
+#define PRU_DEBUG_CT_REG(x)	(0x0080 + (x) * 4)
+
 /**
  * enum pru_mem - PRU core memory range identifiers
  */
@@ -60,6 +65,8 @@  enum pru_mem {
  * @sdram_da: device address of secondary Data RAM for this PRU
  * @shrdram_da: device address of shared Data RAM
  * @fw_name: name of firmware image used during loading
+ * @dbg_single_step: debug state variable to set PRU into single step mode
+ * @dbg_continuous: debug state variable to restore PRU execution mode
  */
 struct pru_rproc {
 	int id;
@@ -74,6 +81,8 @@  struct pru_rproc {
 	u32 sdram_da;
 	u32 shrdram_da;
 	const char *fw_name;
+	u32 dbg_single_step;
+	u32 dbg_continuous;
 };
 
 static void *pru_d_da_to_va(struct pru_rproc *pru, u32 da, int len);
@@ -100,6 +109,130 @@  void pru_debug_write_reg(struct pru_rproc *pru, unsigned int reg, u32 val)
 	writel_relaxed(val, pru->mem_regions[PRU_MEM_DEBUG].va + reg);
 }
 
+static int pru_rproc_debug_read_regs(struct seq_file *s, void *data)
+{
+	struct rproc *rproc = s->private;
+	struct pru_rproc *pru = rproc->priv;
+	int i, nregs = 32;
+	u32 pru_sts;
+	int pru_is_running;
+
+	seq_puts(s, "============== Control Registers ==============\n");
+	seq_printf(s, "CTRL      := 0x%08x\n",
+		   pru_control_read_reg(pru, PRU_CTRL_CTRL));
+	pru_sts = pru_control_read_reg(pru, PRU_CTRL_STS);
+	seq_printf(s, "STS (PC)  := 0x%08x (0x%08x)\n", pru_sts, pru_sts << 2);
+	seq_printf(s, "WAKEUP_EN := 0x%08x\n",
+		   pru_control_read_reg(pru, PRU_CTRL_WAKEUP_EN));
+	seq_printf(s, "CYCLE     := 0x%08x\n",
+		   pru_control_read_reg(pru, PRU_CTRL_CYCLE));
+	seq_printf(s, "STALL     := 0x%08x\n",
+		   pru_control_read_reg(pru, PRU_CTRL_STALL));
+	seq_printf(s, "CTBIR0    := 0x%08x\n",
+		   pru_control_read_reg(pru, PRU_CTRL_CTBIR0));
+	seq_printf(s, "CTBIR1    := 0x%08x\n",
+		   pru_control_read_reg(pru, PRU_CTRL_CTBIR1));
+	seq_printf(s, "CTPPR0    := 0x%08x\n",
+		   pru_control_read_reg(pru, PRU_CTRL_CTPPR0));
+	seq_printf(s, "CTPPR1    := 0x%08x\n",
+		   pru_control_read_reg(pru, PRU_CTRL_CTPPR1));
+
+	seq_puts(s, "=============== Debug Registers ===============\n");
+	pru_is_running = pru_control_read_reg(pru, PRU_CTRL_CTRL) &
+				CTRL_CTRL_RUNSTATE;
+	if (pru_is_running) {
+		seq_puts(s, "PRU is executing, cannot print/access debug registers.\n");
+		return 0;
+	}
+
+	for (i = 0; i < nregs; i++) {
+		seq_printf(s, "GPREG%-2d := 0x%08x\tCT_REG%-2d := 0x%08x\n",
+			   i, pru_debug_read_reg(pru, PRU_DEBUG_GPREG(i)),
+			   i, pru_debug_read_reg(pru, PRU_DEBUG_CT_REG(i)));
+	}
+
+	return 0;
+}
+
+static int pru_rproc_debug_regs_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, pru_rproc_debug_read_regs, inode->i_private);
+}
+
+static const struct file_operations pru_rproc_debug_regs_ops = {
+	.open = pru_rproc_debug_regs_open,
+	.read = seq_read,
+	.llseek	= seq_lseek,
+	.release = single_release,
+};
+
+/*
+ * Control PRU single-step mode
+ *
+ * This is a debug helper function used for controlling the single-step
+ * mode of the PRU. The PRU Debug registers are not accessible when the
+ * PRU is in RUNNING state.
+ *
+ * Writing a non-zero value sets the PRU into single-step mode irrespective
+ * of its previous state. The PRU mode is saved only on the first set into
+ * a single-step mode. Writing a zero value will restore the PRU into its
+ * original mode.
+ */
+static int pru_rproc_debug_ss_set(void *data, u64 val)
+{
+	struct rproc *rproc = data;
+	struct pru_rproc *pru = rproc->priv;
+	u32 reg_val;
+
+	val = val ? 1 : 0;
+	if (!val && !pru->dbg_single_step)
+		return 0;
+
+	reg_val = pru_control_read_reg(pru, PRU_CTRL_CTRL);
+
+	if (val && !pru->dbg_single_step)
+		pru->dbg_continuous = reg_val;
+
+	if (val)
+		reg_val |= CTRL_CTRL_SINGLE_STEP | CTRL_CTRL_EN;
+	else
+		reg_val = pru->dbg_continuous;
+
+	pru->dbg_single_step = val;
+	pru_control_write_reg(pru, PRU_CTRL_CTRL, reg_val);
+
+	return 0;
+}
+
+static int pru_rproc_debug_ss_get(void *data, u64 *val)
+{
+	struct rproc *rproc = data;
+	struct pru_rproc *pru = rproc->priv;
+
+	*val = pru->dbg_single_step;
+
+	return 0;
+}
+DEFINE_SIMPLE_ATTRIBUTE(pru_rproc_debug_ss_fops, pru_rproc_debug_ss_get,
+			pru_rproc_debug_ss_set, "%llu\n");
+
+/*
+ * Create PRU-specific debugfs entries
+ *
+ * The entries are created only if the parent remoteproc debugfs directory
+ * exists, and will be cleaned up by the remoteproc core.
+ */
+static void pru_rproc_create_debug_entries(struct rproc *rproc)
+{
+	if (!rproc->dbg_dir)
+		return;
+
+	debugfs_create_file("regs", 0400, rproc->dbg_dir,
+			    rproc, &pru_rproc_debug_regs_ops);
+	debugfs_create_file("single_step", 0600, rproc->dbg_dir,
+			    rproc, &pru_rproc_debug_ss_fops);
+}
+
 /* start a PRU core */
 static int pru_rproc_start(struct rproc *rproc)
 {
@@ -348,6 +481,8 @@  static int pru_rproc_probe(struct platform_device *pdev)
 		goto free_rproc;
 	}
 
+	pru_rproc_create_debug_entries(rproc);
+
 	dev_info(dev, "PRU rproc node %s probed successfully\n", np->full_name);
 
 	return 0;