| Message ID | 1490466197-29163-1-git-send-email-leo.yan@linaro.org |
|---|---|
| Headers | show |
| Series | coresight: enable debug module | expand |
On Wed, Mar 29, 2017 at 11:07:35AM +0800, Leo Yan wrote: > Hi Suzuki, > > On Mon, Mar 27, 2017 at 05:34:57PM +0100, Suzuki K Poulose wrote: > > On 25/03/17 18:23, Leo Yan wrote: > > [...] > > > Leo, > > > > Thanks a lot for the quick rework. I don't fully understand (yet!) why we need the > > idle_constraint. I will leave it for Sudeep to comment on it, as he is the expert > > in that area. Some minor comments below. > > Thanks a lot for quick reviewing :) > > > >Signed-off-by: Leo Yan <leo.yan@linaro.org> > > >--- > > > drivers/hwtracing/coresight/Kconfig | 11 + > > > drivers/hwtracing/coresight/Makefile | 1 + > > > drivers/hwtracing/coresight/coresight-cpu-debug.c | 704 ++++++++++++++++++++++ > > > 3 files changed, 716 insertions(+) > > > create mode 100644 drivers/hwtracing/coresight/coresight-cpu-debug.c > > > > > >diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig > > >index 130cb21..18d7931 100644 > > >--- a/drivers/hwtracing/coresight/Kconfig > > >+++ b/drivers/hwtracing/coresight/Kconfig > > >@@ -89,4 +89,15 @@ config CORESIGHT_STM > > > logging useful software events or data coming from various entities > > > in the system, possibly running different OSs > > > > > >+config CORESIGHT_CPU_DEBUG > > >+ tristate "CoreSight CPU Debug driver" > > >+ depends on ARM || ARM64 > > >+ depends on DEBUG_FS > > >+ help > > >+ This driver provides support for coresight debugging module. This > > >+ is primarily used to dump sample-based profiling registers when > > >+ system triggers panic, the driver will parse context registers so > > >+ can quickly get to know program counter (PC), secure state, > > >+ exception level, etc. > > > > May be we should mention/warn the user about the possible caveats of using > > this feature to help him make a better decision ? And / Or we should add a documentation > > for it. We have collected some real good information over the discussions and > > it is a good idea to capture it somewhere. > > Sure, I will add a documentation for this. > > [...] > > > >+static struct pm_qos_request debug_qos_req; > > >+static int idle_constraint = PM_QOS_DEFAULT_VALUE; > > >+module_param(idle_constraint, int, 0600); > > >+MODULE_PARM_DESC(idle_constraint, "Latency requirement in microseconds for CPU " > > >+ "idle states (default is -1, which means have no limiation " > > >+ "to CPU idle states; 0 means disabling all idle states; user " > > >+ "can choose other platform dependent values so can disable " > > >+ "specific idle states for the platform)"); > > > > Correct me if I am wrong, > > > > All we want to do is disable the CPUIdle explicitly if the user knows that this > > could be a problem to use CPU debug on his platform. So, in effect, we should > > only be using idle_constraint = 0 or -1. > > > > In which case, we could make it easier for the user to tell us, either > > > > 0 - Don't do anything with CPUIdle (default) > > 1 - Disable CPUIdle for me as I know the platform has issues with CPU debug and CPUidle. > > The reason for not using bool flag is: usually SoC may have many idle > states, so if user wants to partially enable some states then can set > the latency to constraint. > > But of course, we can change this to binary value as you suggested, > this means turn on of turn off all states. The only one reason to use > latency value is it is more friendly for hardware design, e.g. some > platforms can enable partial states to save power and avoid overheat > after using this driver. > > If you guys think this is a bit over design, I will follow up your > suggestion. I also have some replying in Mathieu's reviewing, please > help review as well. > > > than explaining the miscrosecond latency etc and make the appropriate calls underneath. > > something like (not necessarily the same name) : > > > > module_param(broken_with_cpuidle, bool, 0600); > > MODULE_PARAM_DESC(broken_with_cpuidle, "Specifies whether the CPU debug has issues with CPUIdle on" > > " the platform. Non-zero value implies CPUIdle has to be" > > " explicitly disabled.",); > > [...] > > > >+ /* > > >+ * Unfortunately the CPU cannot be powered up, so return > > >+ * back and later has no permission to access other > > >+ * registers. For this case, should set 'idle_constraint' > > >+ * to ensure CPU power domain is enabled! > > >+ */ > > >+ if (!(drvdata->edprsr & EDPRSR_PU)) { > > >+ pr_err("%s: power up request for CPU%d failed\n", > > >+ __func__, drvdata->cpu); > > >+ goto out; > > >+ } > > >+ > > >+out_powered_up: > > >+ debug_os_unlock(drvdata); > > > > Question: Do we need a matching debug_os_lock() once we are done ? > > I have checked ARM ARMv8, but there have no detailed description for > this. I refered coresight-etmv4 code and Mike's pseudo code, ther have > no debug_os_lock() related operations. > > Mike, Mathieu, could you also help confirm this? I'm not strongly opiniated on the usage of the OS lock, hence being a little nonchalent in the coresight-etmv4 driver. > > [...] > > > >+static void debug_init_arch_data(void *info) > > >+{ > > >+ struct debug_drvdata *drvdata = info; > > >+ u32 mode, pcsr_offset; > > >+ > > >+ CS_UNLOCK(drvdata->base); > > >+ > > >+ debug_os_unlock(drvdata); > > >+ > > >+ /* Read device info */ > > >+ drvdata->eddevid = readl_relaxed(drvdata->base + EDDEVID); > > >+ drvdata->eddevid1 = readl_relaxed(drvdata->base + EDDEVID1); > > > > As mentioned above, both of these registers are only need at init time to > > figure out the flags we set here. So we could remove them. > > > > >+ > > >+ CS_LOCK(drvdata->base); > > >+ > > >+ /* Parse implementation feature */ > > >+ mode = drvdata->eddevid & EDDEVID_PCSAMPLE_MODE; > > >+ pcsr_offset = drvdata->eddevid1 & EDDEVID1_PCSR_OFFSET_MASK; > > > > > > >+ > > >+ if (mode == EDDEVID_IMPL_NONE) { > > >+ drvdata->edpcsr_present = false; > > >+ drvdata->edcidsr_present = false; > > >+ drvdata->edvidsr_present = false; > > >+ } else if (mode == EDDEVID_IMPL_EDPCSR) { > > >+ drvdata->edpcsr_present = true; > > >+ drvdata->edcidsr_present = false; > > >+ drvdata->edvidsr_present = false; > > >+ } else if (mode == EDDEVID_IMPL_EDPCSR_EDCIDSR) { > > >+ if (!IS_ENABLED(CONFIG_64BIT) && > > >+ (pcsr_offset == EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32)) > > >+ drvdata->edpcsr_present = false; > > >+ else > > >+ drvdata->edpcsr_present = true; > > > > Sorry, I forgot why we do this check only in this mode. Shouldn't this be > > common to all modes (of course which implies PCSR is present) ? > > No. PCSROffset is defined differently in ARMv7 and ARMv8; So finally we > simplize PCSROffset value : > 0000 - Sample offset applies based on the instruction state (indicated by PCSR[0]) > 0001 - No offset applies. > 0010 - No offset applies, but do not use in AArch32 mode! > > So we need handle the corner case is when CPU runs AArch32 mode and > PCSRoffset = 'b0010. Other cases the pcsr should be present. > > [...] > > Other suggestions are good for me, will take them in next version. > > Thanks, > Leo Yan
On 29/03/17 11:37, Leo Yan wrote: > On Wed, Mar 29, 2017 at 11:31:03AM +0100, Suzuki K Poulose wrote: >> On 29/03/17 11:27, Leo Yan wrote: >>> On Wed, Mar 29, 2017 at 10:07:07AM +0100, Suzuki K Poulose wrote: >>> >>> [...] >>> >>>>>>> + if (mode == EDDEVID_IMPL_NONE) { >>>>>>> + drvdata->edpcsr_present = false; >>>>>>> + drvdata->edcidsr_present = false; >>>>>>> + drvdata->edvidsr_present = false; >>>>>>> + } else if (mode == EDDEVID_IMPL_EDPCSR) { >>>>>>> + drvdata->edpcsr_present = true; >>>>>>> + drvdata->edcidsr_present = false; >>>>>>> + drvdata->edvidsr_present = false; >>>>>>> + } else if (mode == EDDEVID_IMPL_EDPCSR_EDCIDSR) { >>>>>>> + if (!IS_ENABLED(CONFIG_64BIT) && >>>>>>> + (pcsr_offset == EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32)) >>>>>>> + drvdata->edpcsr_present = false; >>>>>>> + else >>>>>>> + drvdata->edpcsr_present = true; >>>>>> >>>>>> Sorry, I forgot why we do this check only in this mode. Shouldn't this be >>>>>> common to all modes (of course which implies PCSR is present) ? >>>>> >>>>> No. PCSROffset is defined differently in ARMv7 and ARMv8; So finally we >>>>> simplize PCSROffset value : >>>>> 0000 - Sample offset applies based on the instruction state (indicated by PCSR[0]) >>>>> 0001 - No offset applies. >>>>> 0010 - No offset applies, but do not use in AArch32 mode! >>>>> >>>>> So we need handle the corner case is when CPU runs AArch32 mode and >>>>> PCSRoffset = 'b0010. Other cases the pcsr should be present. >>>> >>>> I understand that reasoning. But my question is, why do we check for PCSROffset >>>> only when mode == EDDEVID_IMPL_EDPCSR_EDCIDSR and not for say mode == EDDEVID_IMPL_EDPCSR or >>>> any other mode where PCSR is present. >>> >>> Sorry I misunderstood your question. >>> >>> I made mistake when I analyzed the possbile combination for mode and >>> PCSROffset so I thought it's the only case should handle: >>> { EDDEVID_IMPL_EDPCSR_EDCIDSR, EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 } >>> >>> Below three combinations are possible to exist; so you are right, I >>> should move this out for the checking: >>> { EDDEVID_IMPL_NONE, EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 } >> >> That need not be covered, as IMPL_NONE says PCSR is not implemented hence you >> don't worry about anything as the functionality is missing. This should rather be: >> EDDEVID_IMPL_EDPCSR, where only PCSR is implemented. > > I think below combination doesn't really exist: > { EDDEVID_IMPL_EDPCSR, EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 }; > > EDDEVID_IMPL_EDPCSR is only defined in ARMv7 ARM, and > EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32 is only defined in ARMv8 ARM. It is not wrong to check the PCSROffset in all cases where PCSR is available, as if we hit PCSR on ARMv7 then PCSROffset shouldn't be DIS_AARCH32. And in fact that would make the code a bit more cleaner. Anyways, I am not particular about this. Suzuki
On Wed, Mar 29, 2017 at 09:54:23AM +0800, Leo Yan wrote: > Hi Mathieu, > > On Tue, Mar 28, 2017 at 10:50:10AM -0600, Mathieu Poirier wrote: > > On Sun, Mar 26, 2017 at 02:23:14AM +0800, Leo Yan wrote: > > [...] > > > > +static void debug_force_cpu_powered_up(struct debug_drvdata *drvdata) > > > +{ > > > + int timeout = DEBUG_WAIT_TIMEOUT; > > > + > > > + drvdata->edprsr = readl_relaxed(drvdata->base + EDPRSR); > > > + > > > + CS_UNLOCK(drvdata->base); > > > + > > > + /* Bail out if CPU is powered up yet */ > > > + if (drvdata->edprsr & EDPRSR_PU) > > > + goto out_powered_up; > > > + > > > + /* > > > + * Send request to power management controller and assert > > > + * DBGPWRUPREQ signal; if power management controller has > > > + * sane implementation, it should enable CPU power domain > > > + * in case CPU is in low power state. > > > + */ > > > + drvdata->edprsr = readl(drvdata->base + EDPRCR); > > > + drvdata->edprsr |= EDPRCR_COREPURQ; > > > + writel(drvdata->edprsr, drvdata->base + EDPRCR); > > > > Here ->edprsr is used but EDPRCR is accessed. Is this intentional or a > > copy/paste error? The same is true for accesses in the out_powered_up section. > > Thanks for pointing out. This is a typo error and will fix. > > > > + > > > + /* Wait for CPU to be powered up (timeout~=32ms) */ > > > + while (timeout--) { > > > + drvdata->edprsr = readl_relaxed(drvdata->base + EDPRSR); > > > + if (drvdata->edprsr & EDPRSR_PU) > > > + break; > > > + > > > + usleep_range(1000, 2000); > > > + } > > > > See if function readx_poll_timeout() can be used. > > Will use it. > > > > + > > > + /* > > > + * Unfortunately the CPU cannot be powered up, so return > > > + * back and later has no permission to access other > > > + * registers. For this case, should set 'idle_constraint' > > > + * to ensure CPU power domain is enabled! > > > + */ > > > + if (!(drvdata->edprsr & EDPRSR_PU)) { > > > + pr_err("%s: power up request for CPU%d failed\n", > > > + __func__, drvdata->cpu); > > > + goto out; > > > + } > > > + > > > +out_powered_up: > > > + debug_os_unlock(drvdata); > > > + > > > + /* > > > + * At this point the CPU is powered up, so set the no powerdown > > > + * request bit so we don't lose power and emulate power down. > > > + */ > > > + drvdata->edprsr = readl(drvdata->base + EDPRCR); > > > + drvdata->edprsr |= EDPRCR_COREPURQ | EDPRCR_CORENPDRQ; > > > > If we are here the core is already up. Shouldn't we need to set > > EDPRCR_CORENPDRQ only? > > Yeah. Will fix. > > > > + writel(drvdata->edprsr, drvdata->base + EDPRCR); > > > > This section is a little racy - between the time the PU bit has been > > checked and the time COREPDRQ has been flipped, the state of PU may have > > changed. You can probably get around this by checking edprsr.PU rigth here. If > > it is not set you go through the process again. Note that doing this will > > probably force a refactoring of the whole function. > > Agree. Will handle this. > > [...] > > > > +static ssize_t debug_func_knob_write(struct file *f, > > > + const char __user *buf, size_t count, loff_t *ppos) > > > +{ > > > e u8 on; > > > + int ret; > > > + > > > + ret = kstrtou8_from_user(buf, count, 2, &on); > > > + if (ret) > > > + return ret; > > > + > > > + mutex_lock(&debug_lock); > > > + > > > + if (!on ^ debug_enable) > > > + goto out; > > > > I had to read this condition too many times - please refactor. > > Will do it. > > > > + > > > + if (on) { > > > + ret = debug_enable_func(); > > > + if (ret) { > > > + pr_err("%s: unable to disable debug function: %d\n", > > > + __func__, ret); > > > > Based on the semantic this is the wrong error message. > > Yeah. Will fix. > > > > + goto err; > > > + } > > > + } else > > > + debug_disable_func(); > > > > Did checkpatch.pl complain about extra curly braces? If not please add them. > > checkpatch.pl doesn't report for this. Will add. > > > > + > > > + debug_enable = on; > > > > Here we can't set debug_enable if we just called debug_disable_func(). Maybe > > I'm missing something. If that's the case a comment in the code would be worth > > it. > > After called debug_disable_func(), debug_enable is set to 0 (on = 0). > > > > + > > > +out: > > > + ret = count; > > > +err: > > > + mutex_unlock(&debug_lock); > > > + return ret; > > > +} > > > + > > > +static ssize_t debug_func_knob_read(struct file *f, > > > + char __user *ubuf, size_t count, loff_t *ppos) > > > +{ > > > + char val[] = { '0' + debug_enable, '\n' }; > > > + > > > + return simple_read_from_buffer(ubuf, count, ppos, val, sizeof(val)); > > > > Use the debug_lock to avoid race conditions. > > Will do it. > > > > +} > > > + > > > +static ssize_t debug_idle_constraint_write(struct file *f, > > > + const char __user *buf, size_t count, loff_t *ppos) > > > +{ > > > + int val; > > > + ssize_t ret; > > > + > > > + ret = kstrtoint_from_user(buf, count, 10, &val); > > > + if (ret) > > > + return ret; > > > + > > > + mutex_lock(&debug_lock); > > > + idle_constraint = val; > > > + > > > + if (debug_enable) > > > + pm_qos_update_request(&debug_qos_req, idle_constraint); > > > + > > > + mutex_unlock(&debug_lock); > > > + return count; > > > +} > > > + > > > +static ssize_t debug_idle_constraint_read(struct file *f, > > > + char __user *ubuf, size_t count, loff_t *ppos) > > > +{ > > > + char buf[32]; > > > + int len; > > > + > > > + if (*ppos) > > > + return 0; > > > + > > > + len = sprintf(buf, "%d\n", idle_constraint); > > > + return simple_read_from_buffer(ubuf, count, ppos, buf, len); > > > > Use the debug_lock to avoid race conditions. > > Will do it. > > > > +} > > > + > > > +static const struct file_operations debug_func_knob_fops = { > > > + .open = simple_open, > > > + .read = debug_func_knob_read, > > > + .write = debug_func_knob_write, > > > +}; > > > + > > > +static const struct file_operations debug_idle_constraint_fops = { > > > + .open = simple_open, > > > + .read = debug_idle_constraint_read, > > > + .write = debug_idle_constraint_write, > > > +}; > > > + > > > +static int debug_func_init(void) > > > +{ > > > + struct dentry *file; > > > + int ret; > > > + > > > + /* Create debugfs node */ > > > + debug_debugfs_dir = debugfs_create_dir("coresight_cpu_debug", NULL); > > > + if (!debug_debugfs_dir) { > > > + pr_err("%s: unable to create debugfs directory\n", __func__); > > > + return -ENOMEM; > > > > return PTR_ERR(debug_debugfs_dir); > > Here cannot use PTR_ERR(debug_debugfs_dir). If create debugfs failed > the pointer is NULL value, so finally we will return zero value for > PTR_ERR(debug_debugfs_dir). > > [...] > > > > + } > > > + > > > + file = debugfs_create_file("enable", S_IRUGO | S_IWUSR, > > > + debug_debugfs_dir, NULL, &debug_func_knob_fops); > > > + if (!file) { > > > + pr_err("%s: unable to create enable knob file\n", __func__); > > > + ret = -ENOMEM; > > > > Same as above. > > > > > + goto err; > > > + } > > > + > > > + file = debugfs_create_file("idle_constraint", S_IRUGO | S_IWUSR, > > > + debug_debugfs_dir, NULL, &debug_idle_constraint_fops); > > > + if (!file) { > > > + pr_err("%s: unable to create idle constraint file\n", __func__); > > > + ret = -ENOMEM; > > > > Same as above. > > > > > + goto err; > > > + } > > > + > > > + /* Use sysfs node to enable functionality */ > > > + if (!debug_enable) > > > + return 0; > > > + > > > + /* Enable debug module at boot time */ > > > + ret = debug_enable_func(); > > > + if (ret) { > > > + pr_err("%s: unable to disable debug function: %d\n", > > > + __func__, ret); > > > + goto err; > > > + } > > > > Use the debug_lock to avoid race conditions. > > I'm struggling to understand what's race condition at here? The > function pairs debug_func_init()/debug_func_exit() are used for > module's probing and removing, so naturally module's probing and > removing are sequential, right? You are correct - void that comment. > > > > + > > > + return 0; > > > + > > > +err: > > > + debugfs_remove_recursive(debug_debugfs_dir); > > > + return ret; > > > +} > > > + > > > +static void debug_func_exit(void) > > > +{ > > > + debugfs_remove_recursive(debug_debugfs_dir); > > > + > > > + /* Disable functionality if has enabled */ > > > + if (debug_enable) > > > + debug_disable_func(); > > > +} > > > + > > > +static int debug_probe(struct amba_device *adev, const struct amba_id *id) > > > +{ > > > + void __iomem *base; > > > + struct device *dev = &adev->dev; > > > + struct debug_drvdata *drvdata; > > > + struct resource *res = &adev->res; > > > + struct device_node *np = adev->dev.of_node; > > > + int ret; > > > + > > > + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL); > > > + if (!drvdata) > > > + return -ENOMEM; > > > + > > > + drvdata->cpu = np ? of_coresight_get_cpu(np) : 0; > > > + if (per_cpu(debug_drvdata, drvdata->cpu)) { > > > + dev_err(dev, "CPU's drvdata has been initialized\n"); > > > > Might be worth adding the CPU number in the error message. > > Yeah, will add it. > > [...] > > > This driver doesn't call the pm_runtime_put/get() operations needed to handle the > > debug power domain. See the other CoreSight drivers for details. > > Sure, will do it. > > > Also, from the conversation that followed the previous post we agreed that we wouldn't > > deal with CPUidle issues in this driver. We deal with the CPU power domain > > using the EDPRCR register (like you did) and that's it. System that don't honor that register > > can use other (external) means to solve this. As such please remove the > > pm_qos_xyz() functions. > > From previous discussion, Mike reminds the CPU power domain design is > quite SoC specific and usually the SoC has many different low power > states, e.g. except CPU level and cluster level low power states, they > also can define SoC level low power states. Any SoC with any power > state is possible finally impact CPU power domain, so this is why I add > this interface to let user can have the final decision based on their > working platform. Mike is correct but there are other ways to deal with these cases, i.e cpuidle interface from cmd line. > > We can rely on "nohlt" and "cpuidle.off=1" in kernel command line to > disable whole SoC low power states at boot time; or we can use sysfs > node "echo 1 > /sys/devices/system/cpu/cpuX/cpuidle/stateX/disble" to > disable CPU low power states at runtime. But that means we need use > different interfaces to control CPU power domain for booting and > runtime, it's not nice for usage. That is a different topic altogether. > > So this is why add "idle_constraint" as a central place to control > power domain for CPU debug purpose and I also think this is more > friendly for hardware design, e.g. some platforms can enable partial > low power states to save power and avoid overheat after using this > driver. > > How about you think for this? Like Sudeep pointed out we should concentrate on doing the right thing, that is work with EDPRSR.PU, EDPRCR.COREPURQ and EDPRCR.CORENPDRQ. Anything outside of that becomes platform specific and can't be handled in this driver. > > Thanks, > Leo Yan
On 30/03/17 02:03, Leo Yan wrote: > On Wed, Mar 29, 2017 at 03:56:23PM +0100, Mike Leach wrote: > > [...] > >>>>> + /* >>>>> + * Unfortunately the CPU cannot be powered up, so return >>>>> + * back and later has no permission to access other >>>>> + * registers. For this case, should set 'idle_constraint' >>>>> + * to ensure CPU power domain is enabled! >>>>> + */ >>>>> + if (!(drvdata->edprsr & EDPRSR_PU)) { >>>>> + pr_err("%s: power up request for CPU%d failed\n", >>>>> + __func__, drvdata->cpu); >>>>> + goto out; >>>>> + } >>>>> + >>>>> +out_powered_up: >>>>> + debug_os_unlock(drvdata); >>>>> + >>>>> + /* >>>>> + * At this point the CPU is powered up, so set the no powerdown >>>>> + * request bit so we don't lose power and emulate power down. >>>>> + */ >>>>> + drvdata->edprsr = readl(drvdata->base + EDPRCR); >>>>> + drvdata->edprsr |= EDPRCR_COREPURQ | EDPRCR_CORENPDRQ; >>>> >>>> If we are here the core is already up. Shouldn't we need to set >>>> EDPRCR_CORENPDRQ only? >>> >>> Yeah. Will fix. >> >> No - EDPRCR_COREPURQ and EDPRCR_CORENPDRQ have different semantics and purposes >> >> EDPRCR_COREPURQ is in the debug power domain an is tied to an external >> debug request that should be an input to the external (to the PE) >> system power controller. >> The requirement is that the system power controller powers up the core >> domain and does not power it down while it remains asserted. >> >> EDPRCR_CORENPDRQ is in the core power domain and thus to the specific >> core only. This ensures that any power control software running on >> that core should emulate a power down if this is set to one. > > I'm curious the exact meaning for "power control software". > > Does it mean EDPRCR_CORENPDRQ should be checked by kernel or PSCI > liked code in ARM trusted firmware to avoid to run CPU power off flow? > > Or will EDPRCR_CORENPDRQ assert CPU external signal to notify power > controller so power controller emulate a power down? > >> We cannot know the power control design of the system, so the safe >> solution is to set both bits. > > Thanks a lot for the suggestion. Will set both bits. Leo, Also, it would be good to restore the PRCR register back to the original state after we read the registers (if we changed them). I am exploring ways to make use of this feature on demand (e.g, tie it to sysrq-t or sysrq-l) and not just at panic time. So it would be good to have the state restored to not affect the normal functioning even after dumping the registers. PS: I have a small hack to hook this into a sysrq-x at runtime, but on a second thought it is better not to consume the key and rather tie it to something which already exist, as mentioned above. Thanks Suzuki > > Thanks, > Leo Yan >
On Thu, Mar 30, 2017 at 10:00:30AM +0100, Suzuki K Poulose wrote: [...] > Leo, > > Also, it would be good to restore the PRCR register back to the original state > after we read the registers (if we changed them). I am exploring ways to make > use of this feature on demand (e.g, tie it to sysrq-t or sysrq-l) and not just > at panic time. So it would be good to have the state restored to not affect the > normal functioning even after dumping the registers. Got it. Will take care for this. Thanks, Leo Yan > PS: I have a small hack to hook this into a sysrq-x at runtime, but on a second thought > it is better not to consume the key and rather tie it to something which already exist, > as mentioned above. > > Thanks > Suzuki