| Message ID | 1543218769-5507-6-git-send-email-rogerq@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | remoteproc: Add support for TI PRU | expand |
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.
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
+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
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?
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
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?
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.
* 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
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
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;